-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Contacts flow #60
Contacts flow #60
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #60 +/- ##
========================================
- Coverage 5.55% 5.40% -0.16%
========================================
Files 41 42 +1
Lines 3059 3182 +123
Branches 19 59 +40
========================================
+ Hits 170 172 +2
- Misses 2889 3010 +121
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
<Input id="dateOfBirth" label="Date of birth (MM/DD/YYYY" /> | ||
<Select> | ||
<SelectTrigger label="Country of issuing" id="countryOfIssuing"> | ||
<SelectValue placeholder="Select an option" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should placeholders already be localized with FormattedMessage
?
<SelectValue placeholder="Select an option" /> | |
<SelectValue placeholder={<FormattedMessage | |
id="pages.createContact.select.country" | |
defaultMessage="Select an option" | |
description="Placeholder for country selection" | |
/>} /> |
This applies to other non-translated fields/placeholders/etc. in this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, thank you! I'm gonna replace that. In this case, we must use the imperative API so we don't have to change the typedef for the component.
Ref: https://formatjs.github.io/docs/react-intl/api/#useintl-hook
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is manual work necessary when reviewing the screens. It would be nice if the page routes are registered so that one can browse them without changing the code for the review process. e.g. reaching the empty list screen like for notifications (/notifications-empty
) or bills (/bills-empty
). What do you think?
Other than that: Looks nice. Approved. 💪
agree |
We can do that with Storybook. Will add it to the PR. Thanks! |
No description provided.