Email Aliases: error message for GenerateAlias #29153
Conversation
- mojom file definition - updated UI implementation - unit tests - storybook
github-actions
bot
added
the
CI/storybook-url
|
A Storybook has been deployed to preview UI for the latest push |
| @@ -51,7 +51,7 @@ interface EmailAliasesService { | |||
| CancelAuthenticationOrLogout(); | |||
|
|
|||
| // Generate a proposed email alias. | |||
| GenerateAlias() => (string alias_email); | |||
There was a problem hiding this comment.
maybe you could use a union for this? It represents the "only one of these should be present" state a bit better :)
union GenerateAliasResult {
string alias;
string error;
}
...
GenerateAlias() => (GenerateAliasResult result);
There was a problem hiding this comment.
on the cpp side (future PR) we should use struct traits to convert this into base::expected as the union is kind of clunky and base::expected is preferred for error handling
browser/resources/settings/email_aliases_page/tests/email_aliases_modal.test.tsx
Outdated
Show resolved
Hide resolved
| const aliasEmailBox = screen.getByText('emailAliasesAliasLabel') | ||
| .closest('div')?.nextElementSibling |
There was a problem hiding this comment.
hey I worry that this selector is going to be a bit brittle if we change the HTML - maybe we should add a data-test-id to the element?
browser/resources/settings/email_aliases_page/tests/email_aliases_modal.test.tsx
Outdated
Show resolved
Hide resolved
browser/resources/settings/email_aliases_page/tests/email_aliases_modal.test.tsx
Outdated
Show resolved
Hide resolved
browser/resources/settings/email_aliases_page/stories/index.tsx
Outdated
Show resolved
Hide resolved
browser/resources/settings/email_aliases_page/content/email_aliases_modal.tsx
Outdated
Show resolved
Hide resolved
browser/resources/settings/email_aliases_page/tests/email_aliases_modal.test.tsx
Outdated
Show resolved
Hide resolved
browser/resources/settings/email_aliases_page/tests/email_aliases_modal.test.tsx
Outdated
Show resolved
Hide resolved
| const [generateAliasResult, setGenerateAliasResult] = | ||
| React.useState<GenerateAliasResult | null>(null) |
There was a problem hiding this comment.
this is completely a matter of personal preference so up to you but I'd normally type this as:
const [generateAliasResult, setGenerateAliasResult] =
React.useState<GenerateAliasResult>()
// generateAliasResult is `GenerateAliasResult | undefined`and then use undefined instead of null
up to you 😄
There was a problem hiding this comment.
works for me; fixed
| mockEmailAliasesService.generateAlias = jest.fn().mockImplementation( | ||
| () => Promise.resolve('new@brave.com')) | ||
| () => Promise.resolve({ result: { errorMessage: null, aliasEmail } })) |
There was a problem hiding this comment.
@fallaciousreasoning is this the correct mapping from mojo interface for optional? It will still have the property and just be null vs not having the property at all?
There was a problem hiding this comment.
Good catch. Somehow the typescript linter wasn't catching this until I explicitly added the type there. Actually it maps to
/**
* @typedef { {
* aliasEmail: (!string|undefined),
* errorMessage: (!string|undefined),
* } }
*/
export const GenerateAliasResult = {};
So I needed to use undefined rather than null. I have pushed a fix
There was a problem hiding this comment.
I have a vague idea that the property has to be present, it just also has to be undefined.
|
For @brave/string-reviewers-team -- here's a screenshot of the string in use: |
|
Released in v1.81.5 |
Resolves brave/brave-browser#46222