Page MenuHomePhabricator

[native] Introduce a restore button
AcceptedPublic

Authored by tomek on Tue, Nov 12, 6:10 AM.
Tags
None
Referenced Files
F3334526: D13906.id45746.diff
Thu, Nov 21, 6:57 AM
F3333968: D13906.id45847.diff
Thu, Nov 21, 5:39 AM
F3333337: D13906.diff
Thu, Nov 21, 4:05 AM
F3329045: D13906.id45836.diff
Wed, Nov 20, 4:35 PM
F3328826: D13906.id45848.diff
Wed, Nov 20, 3:23 PM
Unknown Object (File)
Wed, Nov 20, 10:23 AM
Unknown Object (File)
Wed, Nov 20, 10:11 AM
Unknown Object (File)
Wed, Nov 20, 4:12 AM
Subscribers

Details

Summary

This button is shown only when the flag is set. When it is shown, the login buttons are replaced by restore.

https://linear.app/comm/issue/ENG-9678/introduce-a-restore-button

Depends on D13905

Restore enabled

qr-new.png (2×1 px, 150 KB)

prompt-new.png (2×1 px, 1 MB)

Restore disabled

prompt-false.png (2×1 px, 1 MB)

Test Plan

Flow, tested that the correct set of buttons is shown depending on the flag.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

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

I think it would be best to tuck the restore button behind the login button.

Basically we would show the QR screen, and then there would be a link below it that says something like "No longer have access to your primary device?"

What do you think?

I think it would be best to tuck the restore button behind the login button.

Basically we would show the QR screen, and then there would be a link below it that says something like "No longer have access to your primary device?"

What do you think?

It sounds like you're proposing a solution where on the initial screen there are two buttons: Sign in and Register. Clicking Sign in opens the QR screen, where there is a No longer have access to your primary device? text. Clicking this link would open the restore screen. Is that correct?

If yes, then it makes a lot of sense to me, because seeing a restore button on the initial screen might be confusing. This solution could be a little more complicated than the current one, because the QR screen is a modal that would modify the state of the logged out modal - but nothing too crazy.

One suggestion I have is to consider whether the whole No longer have access to your primary device? text should be a link. Maybe we can keep it as plain text and introduce a restore button below it? Not sure how this button should look given the design of our QR screen.

It sounds like you're proposing a solution where on the initial screen there are two buttons: Sign in and Register. Clicking Sign in opens the QR screen, where there is a No longer have access to your primary device? text. Clicking this link would open the restore screen. Is that correct?

Yes, exactly!

One suggestion I have is to consider whether the whole No longer have access to your primary device? text should be a link. Maybe we can keep it as plain text and introduce a restore button below it? Not sure how this button should look given the design of our QR screen.

Hmm... the reason I'm thinking of a link is that our restore flow is comparable to a "Forgot password?" flow, and I usually see those accessed by a link.

I think a button would be too distracting, and might come across as part of the QR code flow when it's really a separate thing.

Would your concerns be addressed if we came up with shorter copy? Maybe "Lost your primary device?"

Hmm... the reason I'm thinking of a link is that our restore flow is comparable to a "Forgot password?" flow, and I usually see those accessed by a link.

I think a button would be too distracting, and might come across as part of the QR code flow when it's really a separate thing.

Yeah, that makes sense.

Would your concerns be addressed if we came up with shorter copy? Maybe "Lost your primary device?"

I was more concerned about users being able to notice that a link can be clickable. In the mobile app, we usually use buttons to indicate actions that can be performed. There are some usages of LinkButton, but only in the navigation header where users expect some actions to be possible.

I was more concerned about users being able to notice that a link can be clickable. In the mobile app, we usually use buttons to indicate actions that can be performed. There are some usages of LinkButton, but only in the navigation header where users expect some actions to be possible.

That's a fair concern. It might be possible to improve with color, italics, underline, etc... might be easier to think about if we had a screenshot.

I was more concerned about users being able to notice that a link can be clickable. In the mobile app, we usually use buttons to indicate actions that can be performed. There are some usages of LinkButton, but only in the navigation header where users expect some actions to be possible.

That's a fair concern. It might be possible to improve with color, italics, underline, etc... might be easier to think about if we had a screenshot.

Checked how it looks like with and without underline

link-underline.png (2×1 px, 151 KB)

link.png (2×1 px, 150 KB)

To be honest, I think it looks okay without the underline. What do you think?

I think it would be good to have the link aligned with the bottom of the "safe area" (just above the home pill).

Separately: realizing from these screenshots that we tell the user to scan the QR code with their phone, not specifically their primary device. I suspect they maybe don't even know what a "primary device" is...

Potentially, we could take the following approach:

  1. Replace "Lost your primary device?" with something like "Not logged in on your phone?"
  2. If the QR code screen is opened on mobile, then we should say "logged-in phone" or "primary device" instead of just "phone" in the QR code prompt.
  3. We should probably have some specific error we're able to show if the user scans a QR code with a secondary device.

To be honest, I think it looks okay without the underline. What do you think?

Agree, it is clear enough that this is a link

I think it would be good to have the link aligned with the bottom of the "safe area" (just above the home pill).

link-bottom.png (2×1 px, 151 KB)

Separately: realizing from these screenshots that we tell the user to scan the QR code with their phone, not specifically their primary device. I suspect they maybe don't even know what a "primary device" is...

Potentially, we could take the following approach:

  1. Replace "Lost your primary device?" with something like "Not logged in on your phone?"

Updated on the screenshot

  1. If the QR code screen is opened on mobile, then we should say "logged-in phone" or "primary device" instead of just "phone" in the QR code prompt.

I'll create a separate diff for it

  1. We should probably have some specific error we're able to show if the user scans a QR code with a secondary device.

Created https://linear.app/comm/issue/ENG-9936/show-a-specific-error-when-scanning-a-qr-code-on-a-secondary-device to track

Update the UI. Navigating from the QR screen to the restore flow will be added later in the stack.

Looks great!

native/account/logged-out-modal.react.js
560–572 ↗(On Diff #45836)

Something seems wrong here... these lines that are being removed don't seem to appear on master, and don't seem to appear in the parent diff either

native/qr-code/qr-code-screen.react.js
47 ↗(On Diff #45836)

One last comment:

  • On web, I think the current text "Not logged in on your phone?" makes sense
  • On native, I think I'd want it to read slightly different: "Not logged in on another phone?"

Separately, is the onPress replaced in the next diff? If not we might want to move the no-op function definition to the top-level scope to avoid passing a new one to LinkButton on every render

  1. If the QR code screen is opened on mobile, then we should say "logged-in phone" or "primary device" instead of just "phone" in the QR code prompt.

I'll create a separate diff for it

https://phab.comm.dev/D13946

tomek retitled this revision from [lib] Introduce a restore button to [native] Introduce a restore button.Fri, Nov 15, 10:27 AM
native/account/logged-out-modal.react.js
560–572 ↗(On Diff #45836)

Ah, instead of amending I created a new commit - going to fix it.

native/qr-code/qr-code-screen.react.js
47 ↗(On Diff #45836)

This screen is native-specific and there's no web counterpart. On the web, we don't support the restoration flow, so I don't think it makes sense to introduce the link to the restore flow. If we want it, we should make it behave differently, maybe add some explanation that the user can restore their account on a phone... not sure.

Separately, is the onPress replaced in the next diff? If not we might want to move the no-op function definition to the top-level scope to avoid passing a new one to LinkButton on every render

I'm going to open a new diff where this is replaced. This is more involved and it makes sense to separate them out. Not sure yet whether this new diff will appear before or after this one, but I'm going to update this diff accordingly.

On the web, we don't support the restoration flow, so I don't think it makes sense to introduce the link to the restore flow.

Ahh... yes of course, sorry that should've been obvious 😅

This revision is now accepted and ready to land.Fri, Nov 15, 10:40 AM