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 "in-between" components wrapping regular Input/TextInput. In those components I used state
to 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.
Details
Built app.
Tested app in web and native, both iOS and Android.
Diff Detail
- Repository
- rCOMM Comm
- Branch
- see-password
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Sure!
Overall this looks good but one major thing:
This is a big diff - it does the same but the implementation is different depending on the platform, I think it'll be better to divide this into web and native updates (for now I left some comments only for the web part).
And some inline comments.
web/account/password-input.react.js | ||
---|---|---|
6–7 | nit: can be merged into one | |
10–17 | Copying part props from input.react.js is something I would like to avoid. (eg. what is type there will change in the future?) | |
20 | At first sight, it looks like some sort of magic strings, which is not clear to me, but I get this is a prop to standard HTML <input>. I am not sure what will be better, eg. adding special type for this: const passwordInputState= Object.freeze({ passwordHidden: 'password', passwordVisible: 'text', }); export type PasswordInputState = $Values<typeof passwordInputType>; will improve complexity so probably it's better to leave this as it is. | |
21–23 | We are in a functional component, each time component will re-render function will be redefined, we could improve this by using React.useCallback | |
25–27 | Looks like there is no need to use a function - you can use a simple constant here (probably wrapped inside React.useMemo). | |
31 | Let's move the spreading operator to the first place to prevent overriding type (if you think there will be a use case in the future where placeholder will be overridden from the parent component it's okay to leave it before spreading). type={type} can be shortened to type | |
35–37 | nit: shorthand |
web/account/password-input.react.js | ||
---|---|---|
35 | I don't think it's the best pattern to add onClick to <div>, we recently added a generic <Button> component, do you think we can use this here? |
Thanks for extensive review @kamil. I responded to inline comments.
Also deleted native files from this diff and will put them in another one.
web/account/password-input.react.js | ||
---|---|---|
20 | I will leave it as it is, but I have changed names to htmlInputType and setHtmlInputType so it is more clear what the variable is. |
Almost there, requesting changes because of the padding thing but other than that good job!
Should we clear username and password after unsuccessful login?
Personally, from user perspective I like when on unsuccessful login password is wiped out and the username stays but I think you need an opinion from someone more experienced in UX terms.
web/account/password-input.css | ||
---|---|---|
3–4 ↗ | (On Diff #17935) | in case of 0px unit is redundant |
22 ↗ | (On Diff #17935) | I am not sure there is a need to add Wrapper suffix everywhere, we could e.g. rename fieldsetWrapper to wrapper since it's the main responsibility and find a more appropriate name for others but it's up to you. |
web/account/password-input.react.js | ||
35 ↗ | (On Diff #17935) | More often in codebase, I've seen passing size as a number |
20 | sounds good to me | |
web/modals/input.css | ||
23 ↗ | (On Diff #17935) | According to line 14 in this file, this padding will be applied to both password and text input (where there is no icon), is it an intentional change? If now I would suggest apply this padding to place exactly where we have input + icon which is password-input.react.js For me, it's confusing because I will see a wrapping login and an empty space on the right side. |
web/account/password-input.react.js | ||
---|---|---|
12 ↗ | (On Diff #17935) | in InputProps those fields are read-only (+) |
web/account/password-input.react.js | ||
---|---|---|
12 ↗ | (On Diff #17935) | I wonder if this should be defined in the reverse direction, with PasswordInput listing its props, and InputProps being defined as those props + some other props |
web/account/password-input.css | ||
---|---|---|
22 ↗ | (On Diff #17935) | good point, renamed them |
web/account/password-input.react.js | ||
12 ↗ | (On Diff #17935) | I didn't want to list PasswordInput props, as they are already defined in Input which is more widely used in codebase. This essentially represents rest operation I perform later in code. |
web/modals/input.css | ||
23 ↗ | (On Diff #17935) | Good catch, moved it, so it only applies to password input |
Responded to inline comments. After discussion with @tomek I added BaseInputProps = PasswordProps and created InputProps as its extension.
web/account/password-input.css | ||
---|---|---|
11 ↗ | (On Diff #18051) | This selector is probably not what you intended. Instead of selecting .wrapper when in hover state, it selects all the descendants of .wrapper which are :hover. If it was intentional, please explain why we're doing something like this, as this might be bad for maintainability. |
24 ↗ | (On Diff #18051) | Why do we need this? |
web/account/password-input.react.js | ||
13 ↗ | (On Diff #18051) | We can be more specific with the type |
17 ↗ | (On Diff #18051) | Do we really need to use setHtmlInputType as a dependency? Does this ever change? |
20 ↗ | (On Diff #18051) | What is the benefit of memoizing this value? |
web/modals/input.react.js | ||
19–20 ↗ | (On Diff #18051) | I'm not sure if these two need to be mandatory. Can we make them optional? |
45 ↗ | (On Diff #18051) | We should use classNames library to merge classes |
web/account/password-input.css | ||
---|---|---|
11 ↗ | (On Diff #18051) | No, it wasn't intentional. Actually it was confusing me why I had to setup hover in a wrapper component, but it worked. (apparently it was hover on child element). |
24 ↗ | (On Diff #18051) | We don't. For some reason, I couldn't make it work without fieldset, but now it works with div that has position: relative just fine |
web/account/password-input.react.js | ||
17 ↗ | (On Diff #18051) | No, it's a bad habbit from misconfigured eslint in the past. Fixed now |
20 ↗ | (On Diff #18051) | Not a huge one, but if my understanding is correct, we do not need to recalculate if statement on every props change, but only on state change i.e. when user click button. Expression inside is so simple though that maybe we can drop useMemo, so we don't waste memory, as that re-render doesn't happen too often anyway. What would be preferable? |
web/modals/input.react.js | ||
19–20 ↗ | (On Diff #18051) | We could, but I figured if they were mandatory in the original props type, I'd leave them as such. If I should change them, let me know. |
web/account/password-input.react.js | ||
---|---|---|
20 ↗ | (On Diff #18051) | I think that using memo in this case decreases performance: even if we're avoiding an explicit if, the dependencies still need to be compared, so there's likely another if involved. We should use memo if we want to avoid expensive computation or when we want to keep referential equality (not generating new arrays / objects / functions on every render). None of these apply here. |