Details

Reviewers

angelika
bartek
ashoat
kamil

Commits

rCOMM7d5d716deae4: [native] Introduce a restore screen

Summary

Add a new screen where a user can restore the account using a password or SIWE.

https://linear.app/comm/issue/ENG-9679/create-a-screen-where-user-can-choose-between-usernamepassword-and

The restore screen

restore-screen-4.12.png (2×1 px, 181 KB)

The prompt screen after this diff - no changes

Depends on D14066

Test Plan

Checked that this screen looks correctly and clicking the actions navigates to appropriate screens.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

tomek created this revision.Tue, Nov 12, 6:15 AM

Herald added a subscriber: ashoat. · View Herald TranscriptTue, Nov 12, 6:15 AM

tomek edited the summary of this revision. (Show Details)Tue, Nov 12, 6:15 AM

Harbormaster completed remote builds in B32613: Diff 45747.Tue, Nov 12, 7:07 AM

tomek requested review of this revision.Tue, Nov 12, 7:07 AM

Update the padding

tomek edited the summary of this revision. (Show Details)Tue, Nov 12, 7:21 AM

Format

I'm not sure the "OR" thing is necessary anymore now that we only have two buttons instead of three
Can we replace "Sign in" with "Sign in with password"?

Harbormaster completed remote builds in B32615: Diff 45751.Tue, Nov 12, 7:41 AM

Harbormaster completed remote builds in B32614: Diff 45750.Tue, Nov 12, 7:50 AM

Update copy and remove "or" section

tomek edited the summary of this revision. (Show Details)Wed, Nov 13, 7:09 AM

Harbormaster completed remote builds in B32651: Diff 45797.Wed, Nov 13, 7:26 AM

Rebase

Harbormaster completed remote builds in B32678: Diff 45840.Fri, Nov 15, 10:23 AM

tomek added a child revision: D13946: [native] Update the QR login screen to mention the primary device.Fri, Nov 15, 10:24 AM

kamil accepted this revision.Tue, Nov 19, 1:33 AM

This revision is now accepted and ready to land.Tue, Nov 19, 1:33 AM

tomek removed a child revision: D13946: [native] Update the QR login screen to mention the primary device.Tue, Nov 26, 8:16 AM

tomek added a child revision: D13946: [native] Update the QR login screen to mention the primary device.Tue, Nov 26, 8:17 AM

tomek removed a child revision: D13946: [native] Update the QR login screen to mention the primary device.Tue, Nov 26, 8:20 AM

Rebase

tomek added a child revision: D14038: [native] Navigate from the QR screen to the restore screen.Tue, Nov 26, 8:28 AM

Harbormaster completed remote builds in B32874: Diff 46089.Tue, Nov 26, 10:03 AM

tomek edited the summary of this revision. (Show Details)Fri, Nov 29, 4:06 AM

tomek added a parent revision: D14066: [native] Extract the buttons from the logged out modal.

tomek removed a parent revision: D13906: [native] Update the logged out modal.

Introduce a new screen

tomek edited the summary of this revision. (Show Details)Fri, Nov 29, 4:08 AM

tomek retitled this revision from [lib] Introduce a restore screen to [native] Introduce a restore screen.

tomek added inline comments.Fri, Nov 29, 4:14 AM

native/account/restore-prompt-screen.react.js
35 ↗	(On Diff #46130)	This is a kind of a hack. We're using `PromptButton` here and on the logged-out modal. In the modal, we're displaying the buttons inside an absolutely-positioned container that contains more containers with flex-direction `row`. In order for the buttons to share the space evenly, we specify `flex: 1` for them. This doesn't play well with the buttons here, which are displayed in a container that shrinks - the result is that the buttons collapse and take as little space as possible. There are a couple of possible solutions: Specifying `flex-shrink`: 0 for buttons - this breaks the logged out modal (a row with two sign-in buttons) Avoid specifying `flex: 1` for buttons - also breaks the modal Specifying `flex: 1` conditional for the buttons - this works, but the fact that we would need to specify a flag which determines whether we should add `flex: 1` doesn't sound great The solution from this diff, where we artificially introduce a row container for each button I'm not sure whether the solution from this diff is the best. Maybe there are some better alternatives.

tomek requested review of this revision.Fri, Nov 29, 4:14 AM

Harbormaster completed remote builds in B32899: Diff 46130.Fri, Nov 29, 4:24 AM

tomek added inline comments.Fri, Nov 29, 10:24 AM

native/account/restore-prompt-screen.react.js
55–58 ↗	(On Diff #46130)	This isn't used

tomek added a child revision: D14067: [native] Introduce a password account restore screen.Fri, Nov 29, 10:40 AM

I think @ashoat should review this to see the user-facing copy

Some quick UI requests:

Would it be possible to have a graphic on this screen? I like this free SVG, but we could also pick something from @expo/vector-icons:
As compared with similar registration screens, the screenshot here seems to have a lot more margin between the bottom button and the home pill. Can you try to make it look more like the screens in RegistrationNavigator?
Regarding the copy, I'd like to make some updates. In the screenshot it feels like a lot of text bunched together, so I tried to split it up into multiple paragraphs. Would be curious to see if this looks good in an updated screenshot:

If you've lost access to your primary device, you can try recovering your Comm account.

To proceed, select the same login method that you used during registration.

Note that after completing the recovery flow, you will logged out of all of your other devices.

native/account/restore-prompt-screen.react.js
35 ↗	(On Diff #46130)	I wonder if it would be better to implement a separate set of buttons based on `PrimaryButton` for this flow (that's what the `RegistrationNavigator` screens usually use) Potentially we could have some shared inner component that is used by both the `LoggedOutModal` buttons and these buttons, but they would be wrapped in slightly different ways that work in each use case

This revision now requires changes to proceed.Mon, Dec 2, 10:00 AM

In D13907#392115, @ashoat wrote:

Would it be possible to have a graphic on this screen? I like this free SVG, but we could also pick something from @expo/vector-icons:

I spoke to Rahul on our team, and he's interested in helping here a bit. He's going to see if he can find or design a better graphic than the ones that I found.

EDIT: tracked here

Update the design

tomek edited the summary of this revision. (Show Details)Wed, Dec 4, 9:48 AM

A new revision with a couple of changes:

Added a placeholder icon. Will update the diff after https://linear.app/comm/issue/OPS-305/mockup-icon-for-restore-account-screen finalizes the design.
Made sure the margin isn't that big. This issue was caused by the SafeAreaView being rendered twice.
Updated the copy with the text proposed by @ashoat . The first two paragraphs are almost exactly the same - the only difference is the usage of ’ sign. The third paragraph is a little different - I'm not sure but I think that Note that after completing the recovery flow, you will logged out of all of your other devices. was a bit incorrect.
Used the updated PromptButton that follows a design from the registration screen, e.g. in terms of the font.

Harbormaster completed remote builds in B32928: Diff 46212.Wed, Dec 4, 10:09 AM

tomek removed a child revision: D14038: [native] Navigate from the QR screen to the restore screen.Thu, Dec 5, 3:02 AM

Lovely!!

This revision is now accepted and ready to land.Thu, Dec 5, 10:07 PM

Added a placeholder icon. Will update the diff after https://linear.app/comm/issue/OPS-305/mockup-icon-for-restore-account-screen finalizes the design.

I think we have something finalized! But it's worth testing it on light / dark mode to make sure it looks good (assuming we need to support both light and dark mode here... not sure if that's true)

Use the new icon

The dark mode