Paths

Table of Contentst

[native] implement screen after secondary device is connected
ClosedPublic
Actions

Authored by kamil on Thu, Dec 12, 11:10 AM.

Details

Reviewers

ashoat
tomek

Commits

rCOMMc50663f4ab07: [native] implement screen after secondary device is connected

Summary

ENG-9959.

The final screen when adding a secondary device succeeded. There is no option to go back because it could be strange for the user to back to the connecting screen.

In ENG-9968 there is an example of how this looks like

Depends on D14116

Test Plan

Navigate to screen

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

kamil created this revision.Thu, Dec 12, 11:10 AM

kamil held this revision as a draft.

kamil edited the summary of this revision. (Show Details)Thu, Dec 12, 11:11 AM

kamil added inline comments.Thu, Dec 12, 11:15 AM

native/account/qr-auth/qr-auth-navigator.react.js
57	The alternative is to use `headerShown: false,` but then we'd need to add `SafeAreaView` with `top` edge because `RegistrationContainer` has only the `bottom` - not sure which is preferred. If `SafeAreaView` is better, just wrap what is in `SecondaryDeviceConnected` in `SafeAreaView` or make `RegistrationContainer` configurable?

kamil added a child revision: D14150: [native] add danger option to `PrimaryButton`.Thu, Dec 12, 11:20 AM

Harbormaster completed remote builds in B33063: Diff 46384.Thu, Dec 12, 11:28 AM

kamil published this revision for review.Thu, Dec 12, 11:57 AM

Rahul has a graphic almost ready for this! We can either amend it here, or handle it later if it takes a bit before he can deliver the final version.

Requesting changes because the whitespace at the top of the screen looks strange. My first instinct is to find a way to avoid it, but I'm not sure if that will also look weird because the header appears in a different place for this screen vs. the prior one. Curious for @kamil's and @tomek's takes

native/account/qr-auth/qr-auth-navigator.react.js
57	I think it's visually strange that this screen has so much unused whitespace at the top Is there a way to avoid that?
native/account/qr-auth/secondary-device-connected.react.js
49	Let's go with: Your new device has been successfully registered!

This revision now requires changes to proceed.Thu, Dec 12, 12:17 PM

tomek added inline comments.Fri, Dec 13, 2:23 AM

native/account/qr-auth/qr-auth-navigator.react.js
57	I think it might be helpful to see a video of a transition from a screen with a back button to this one. We can compare this design with a solution where the header is placed higher (by disabling a header completely, not just the left part of it).

kamil requested review of this revision.Mon, Dec 16, 2:45 AM

kamil added inline comments.

native/account/qr-auth/qr-auth-navigator.react.js
57	Solution 1, implemented here with `headerLeft: null` Screen: Flow: Solution 2, what I described above with `headerShown: false` and making `RegistrationContainer` configurable to support `top` as a safe area: Screen: Flow: Requesting review to get feedback before updating the code

kamil attached a referenced file: F3472504: ScreenRecording_12-16-2024 11-36-42_1.MP4. (Show Details)Mon, Dec 16, 2:46 AM

kamil attached a referenced file: F3472503: IMG_4823.PNG. (Show Details)

kamil attached a referenced file: F3472484: ScreenRecording_12-16-2024 11-28-59_1.MP4. (Show Details)

kamil attached a referenced file: F3472480: IMG_4788 (1) (1).PNG. (Show Details)

Thanks for the videos, @kamil. I guess I'm leaning towards the existing code now... the whitespace seems less confusing in the video. Curious for @kamil and @tomek's takes, but if you guys agree we can keep it as-is.

Before landing, please make sure to adjust the copy I pointed out in an inline comment!

This revision is now accepted and ready to land.Mon, Dec 16, 6:03 AM

In D14149#394407, @ashoat wrote:

Thanks for the videos, @kamil. I guess I'm leaning towards the existing code now... the whitespace seems less confusing in the video. Curious for @kamil and @tomek's takes, but if you guys agree we can keep it as-is.

Agree, keeping it as is makes sense to me.

adjust the copy

kamil edited the summary of this revision. (Show Details)Tue, Dec 17, 5:02 AM

Harbormaster completed remote builds in B33160: Diff 46539.Tue, Dec 17, 5:06 AM

Closed by commit rCOMMc50663f4ab07: [native] implement screen after secondary device is connected (authored by kamil). · Explain WhyTue, Dec 17, 7:06 AM

This revision was automatically updated to reflect the committed changes.

kamil added a commit: rCOMMc50663f4ab07: [native] implement screen after secondary device is connected.