Details

Reviewers

tomek
atul
marcin
kamil
ginsu

Commits

rCOMMc4ef42a3fb4d: [native] Added show/hide password button.

Summary

Linear issue: https://linear.app/comm/issue/ENG-2050/add-see-password-button-on-the-login-page-and-dont-clear-text-inputs
I've added show/hide password button on tight side of password input in login screen.
I've added new component PasswordInput wrapping regular TextInput.
In PasswordInput I used stateto keep track of button state.
Question:
Should we clear username and password after unsuccessful login?
If we have "show password" as an option I don't think it's necessary, but it's up to you.

Test Plan

Built app.
Tested native app in simulators, both iOS and Android.

Diff Detail

Repository

rCOMM Comm

Branch

see-password

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• przemek created this revision.Oct 27 2022, 2:03 AM

Herald added a subscriber: • abosh. · View Herald TranscriptOct 27 2022, 2:03 AM

• przemek edited the test plan for this revision. (Show Details)Oct 27 2022, 2:05 AM

• przemek added 1 blocking reviewer(s): tomek.

• przemek edited the summary of this revision. (Show Details)

• przemek attached a referenced file: F208612: Screen Recording 2022-10-21 at 14.34.34.mov. (Show Details)

• przemek attached a referenced file: F208613: Screen Recording 2022-10-21 at 14.33.54.mov. (Show Details)

Harbormaster completed remote builds in B13062: Diff 17936.Oct 27 2022, 2:16 AM

• przemek requested review of this revision.Oct 27 2022, 2:16 AM

Where is the diff title?

• przemek retitled this revision from [native] to [native] Added show/hide password button..Oct 27 2022, 7:19 AM

@kamil could you review this when you have time?
@przemek I think it makes sense to reference D5453. It is not strictly a parent of this diff, but I think adding it as a parent would make the review easier.

• przemek added a parent revision: D5453: [web] Added show/hide password button..Oct 28 2022, 4:01 AM

• przemek added a reviewer: ginsu.Oct 28 2022, 10:36 AM

Looks good to me!

I think here we also need to consider adding this icon on the signup modal, can you mention this in the current task or create another one to discuss if we need this?

native/account/password-input.react.js
24 ↗	(On Diff #17936)	maybe let's make it more similar to D5453 because it's the same functionality and name it `onToggleShowPassword`?
75 ↗	(On Diff #17936)	I feel like this button should be a little closer to the right side, don't you think?

tomek requested changes to this revision.Nov 2 2022, 7:24 AM

tomek added inline comments.

native/account/log-in-panel.react.js
137 ↗	(On Diff #17936)	We shouldn't use arrays directly as props - they would be recomputed on every render. Let's use memo instead
350 ↗	(On Diff #17936)	It feels like this should be a part of `PasswordInput` - it depends on the size of `eye` icon, while `LogInPanel` shouldn't even know that there's an icon at all.
native/account/password-input.react.js
17 ↗	(On Diff #17936)	I think it would be easier to have a functional component. It's probably not worth to change it now, but curious about the reason behind it.
40–43 ↗	(On Diff #17936)	Why do we need these props. Do we use similar props in other places?
56–60 ↗	(On Diff #17936)	Not sure, but seems like this can be written simpler
63–64 ↗	(On Diff #17936)	I know we're using invariants in a lot of places, but we should avoid using them unless necessary. In this case a simple `if` (or even optional chaining) should be enough.
74 ↗	(On Diff #17936)	Is there a way to avoid absolute positioning? It should be possible to position input and button inside their parent using flex.

This revision now requires changes to proceed.Nov 2 2022, 7:24 AM

tomek added inline comments.Nov 2 2022, 9:52 AM

native/account/password-input.react.js
49 ↗	(On Diff #17936)	Not sure but maybe something from `useColors` might work?

Responded to inline comments.

native/account/log-in-panel.react.js
137 ↗	(On Diff #17936)	Moving to other component and leaving as it is. After discussion with @tomek we found that task should be created to fix it in other components as that pattern is widely used in out codebase.
native/account/password-input.react.js
17 ↗	(On Diff #17936)	Both `LogInPanel` and `TextInput` are class components, and I found it more clear that `PasswordInput` is also Class as it share a lot of same elements with them that would need to be rewritten in class component
40–43 ↗	(On Diff #17936)	In some places we do, in some places we don't. It changes visuals of the button. Without those four lines it looks like this: It still looks very good so I think we go with it.
49 ↗	(On Diff #17936)	In `log-in-panel.react.js` we also use hardcoded #555. I think the reason is it is impossible to call useColors in class component. If there is a way to solve it, let me know.
56–60 ↗	(On Diff #17936)	When trying your solution I get Cannot build a typed interface for this module. You should annotate the exports of this module with types. Missing type annotation at property `wrappedTextInputRef` So I think sticking to full annotation is correct.
74 ↗	(On Diff #17936)	I think it may be hard, as we want Button to be overlayed over TextInput. I couldn't really style it in any other way and I think it is acceptable here. If I should purse relative styling here let me know.

• przemek attached a referenced file: F218886: Screen Recording 2022-11-03 at 15.22.55.mov. (Show Details)Nov 3 2022, 8:07 AM

Harbormaster completed remote builds in B13155: Diff 18052.Nov 3 2022, 8:21 AM

ashoat added inline comments.Nov 3 2022, 12:20 PM

native/account/password-input.react.js
75 ↗	(On Diff #17936)	Can we get updated visuals to see how it looks after the button was moved rightward?

• przemek marked an inline comment as done.Nov 4 2022, 1:21 AM

• przemek added inline comments.

native/account/password-input.react.js
75 ↗	(On Diff #17936)	Here you go. It's hard to judge how for should it be, but I think we can leave it to taste of, now present, design team

• przemek attached a referenced file: F220995: Screen Recording 2022-11-04 at 09.20.38.mov. (Show Details)Nov 4 2022, 1:22 AM

tomek accepted this revision.Nov 4 2022, 4:16 AM

tomek added inline comments.

native/account/password-input.react.js
17–18 ↗	(On Diff #18052)	Just a nit: please add an empty line between fields and the constructor
75 ↗	(On Diff #18052)	When exporting a single component, we should prefer default exports