Details
Navigate to screen
Diff Detail
- Repository
- rCOMM Comm
- Branch
- land
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| native/account/qr-auth/qr-auth-navigator.react.js | ||
|---|---|---|
| 57 ↗ | (On Diff #46384) | 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? |
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 ↗ | (On Diff #46384) | 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 ↗ | (On Diff #46384) | Let's go with:
|
| native/account/qr-auth/qr-auth-navigator.react.js | ||
|---|---|---|
| 57 ↗ | (On Diff #46384) | 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). |
| native/account/qr-auth/qr-auth-navigator.react.js | ||
|---|---|---|
| 57 ↗ | (On Diff #46384) | Solution 1, implemented here with headerLeft: null  Flow: Solution 2, what I described above with headerShown: false and making RegistrationContainer configurable to support top as a safe area:  Flow: Requesting review to get feedback before updating the code |
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!