Page MenuHomePhabricator
Differential D14159

[native] replace back button with close button on the scanning QR code screen
ClosedPublic
Actions

Authored by kamil on Mon, Dec 16, 3:27 AM.
Tags
None
Referenced Files
F3485173: D14159.id46554.diff
Tue, Dec 17, 6:50 PM
F3485172: D14159.id46542.diff
Tue, Dec 17, 6:50 PM
F3485171: D14159.id46458.diff
Tue, Dec 17, 6:50 PM
F3485154: D14159.id.diff
Tue, Dec 17, 6:49 PM
F3485147: D14159.diff
Tue, Dec 17, 6:48 PM
F3473000: IMG_4827.PNG
Mon, Dec 16, 3:29 AM
Subscribers
None

Details

Reviewers
ashoat
tomek
Commits
rCOMM7c658ac97e62: [native] replace back button with close button on the scanning QR code screen
Summary

Part of ENG-9959.

Tracked in ENG-10026.

IMG_4827.PNG (1×828 px, 2 MB)

Depends on D14153

Test Plan

Test button

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

kamil created this revision.Mon, Dec 16, 3:27 AM
kamil held this revision as a draft.
kamil edited the summary of this revision. (Show Details)Mon, Dec 16, 3:29 AM
kamil added inline comments.
native/account/qr-auth/qr-code-scanner-left-button.react.js
25–31 ↗(On Diff #46458)

Inspired by close button in CameraModal but instead of Text x I used icon.

kamil published this revision for review.Mon, Dec 16, 3:40 AM
Harbormaster completed remote builds in B33117: Diff 46458.Mon, Dec 16, 3:44 AM
ashoat accepted this revision.Mon, Dec 16, 6:10 AM
ashoat added inline comments.
native/account/qr-auth/qr-code-scanner-left-button.react.js
10–12 ↗(On Diff #46458)

It's a little weird that we're passing all of these props in but only using one. Should we just have a single prop instead? In that case we could potentially reuse this component, such as in CameraModal

25–31 ↗(On Diff #46458)

In CameraModal we actually use the text ×, not the text x. I'm okay with the icon instead, but I think either option is fine

This revision is now accepted and ready to land.Mon, Dec 16, 6:10 AM
kamil added a child revision: D14161: [native] update `PrimaryButton` to handle light theme.Tue, Dec 17, 2:41 AM
kamil updated this revision to Diff 46542.Tue, Dec 17, 6:26 AM

address review

kamil added inline comments.Tue, Dec 17, 6:32 AM
native/navigation/header-close-left-button.react.js
10–32 ↗(On Diff #46542)

This component is implemented like this because it is prepared to be used in CameraModal too (here).

I was struggling to make it work with and without forwardRef so for now I am creating a follow-up task to address this later and to not block the signed device list workstream: ENG-10029.

kamil added inline comments.Tue, Dec 17, 6:33 AM
native/navigation/header-close-left-button.react.js
11 ↗(On Diff #46542)

This has to be optional because this is how it is defined in StackHeaderLeftButtonProps

Harbormaster failed remote builds in B33163: Diff 46542!Tue, Dec 17, 6:46 AM
Harbormaster completed remote builds in B33163: Diff 46542.Tue, Dec 17, 7:02 AM
Closed by commit rCOMM7c658ac97e62: [native] replace back button with close button on the scanning QR code screen (authored by kamil). · Explain WhyTue, Dec 17, 7:06 AM
This revision was automatically updated to reflect the committed changes.
kamil added a commit: rCOMM7c658ac97e62: [native] replace back button with close button on the scanning QR code screen.

Revision Contents
Changeset List

PathSize
native/
account/
qr-auth/
11 lines
navigation/
34 lines

Diff 46554

View Options

native/account/qr-auth/qr-auth-navigator.react.js

Loading...
View Options

native/navigation/header-close-left-button.react.js

Loading...