Conversation
|
|
||
| // TODO, actually get this number back and format it | ||
| const formattedFullPhoneNumber = '+1 (555) 555-5555'; | ||
| const formattedFullPhoneNumber = '+1 ••• ••••'; |
There was a problem hiding this comment.
I think auth client supports fetching this value? Is there a reason not to do this here?
There was a problem hiding this comment.
I was hoping to rebase on Lauren PR, but I can do as follow up in #18287 too.
There was a problem hiding this comment.
I think we want the full number here instead of just last four digits (based on Figma), so that might be a separate request? I'm still on the fence about this. Kinda feel like we could display the whole number all the time in Settings when the session is 2FA verified.
There was a problem hiding this comment.
@vbudhram I can fix this in my PR since I have some similar fixes. I'm going to have merge conflicts with your GQL recovery phone update that's in the demo branch, so I'll rebase after this lands and get it sorted.
There was a problem hiding this comment.
Just a heads up I'm updating this in my branch, but I'm just going to just display the unformatted phone number. I'm going to file a follow up issue from my PR around what we get back when the session isn't verified + that national_format bit so it should get taken care of there.
vbudhram
force-pushed
the
fxa-10373
branch
3 times, most recently
from
a6310ff to
d8d0432
Compare
vpomerleau
left a comment
vpomerleau
left a comment
There was a problem hiding this comment.
Noted a few comments. Might be good to update some before merge, but I won't block on them because we have opportunities to adjust in a few other PRs including the other one you have in flight.
| const user = userEvent.setup(); | ||
| await act(async () => { | ||
| await user.click(screen.getByRole('link', { name: 'Cancel' })); | ||
| }); |
There was a problem hiding this comment.
missing assertion to verify what happens after clicking?
There was a problem hiding this comment.
I'm not sure how to test this since page navigates with
<Link className="link-blue text-sm" to={SETTINGS_PATH}>
Cancel
</Link>
There was a problem hiding this comment.
I see, might better suited for a functional test then!
|
|
||
| // TODO, actually get this number back and format it | ||
| const formattedFullPhoneNumber = '+1 (555) 555-5555'; | ||
| const formattedFullPhoneNumber = '+1 ••• ••••'; |
There was a problem hiding this comment.
I think we want the full number here instead of just last four digits (based on Figma), so that might be a separate request? I'm still on the fence about this. Kinda feel like we could display the whole number all the time in Settings when the session is 2FA verified.
5 participants
Because
This pull request
Issue that this pull request solves
Closes: https://mozilla-hub.atlassian.net/browse/FXA-10373
Checklist
Other information (Optional)
Any other information that is important to this pull request.