Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(web): Added password field visibility toggle #7368

Merged
merged 5 commits into from
Feb 24, 2024

Conversation

Jan108
Copy link
Contributor

@Jan108 Jan108 commented Feb 23, 2024

Added a toggle button to password fields, namely:

  • Login
  • Create User
  • Change Password
  • Change Password after first login
  • Admin register

First step for discussion: #5855

Screenshots

Login with hidden password
Login with visible password

@waclaw66
Copy link
Contributor

Wouldn't be better to use eye-off-outline instead of eye-closed?
obrazek

@michelheusschen
Copy link
Contributor

Thanks for the PR and nice convenience feature, however I have a few suggestions for improvement:

  • Keep <label> out of the PasswordField component, for better reusability
  • Use the mdiEyeOffOutline icon as already suggested
  • Use Tailwind classes instead of custom CSS and inline styles
  • Only show toggle button when password is entered
  • Element IDs should be unique, now every password input has id="password"

As the list has gotten pretty long, I went ahead and pushed some changes. Feel free to make further adjustments as you see fit.

Copy link
Member

@danieldietzler danieldietzler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated but might be worth adding a password input component, since at the moment those "Passwords do not match" error messages + their handling is duplicated

autocomplete={passwordAutocomplete}
{required}
password={value.toString()}
onInput={(passwordValue) => (value = passwordValue)}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We really should bind value.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit tricky, just doing bind:password={value} won't work because value in this component also handles numbers

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh. I missed that, sorry...

Copy link
Contributor

@jrasm91 jrasm91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@danieldietzler
Copy link
Member

Unrelated but might be worth adding a password input component, since at the moment those "Passwords do not match" error messages + their handling is duplicated

@michelheusschen did you take note of that comment as well? What's your opinion about this?

@michelheusschen
Copy link
Contributor

Validating forms in general is not done very uniformly and I think that improvements can certainly be made there. Once that's done, I'm not sure if a password confirmation component is necessary. I don't think the duplication is so bad as to be a problem, but if we decide to address it let's do it in a separate PR.

@alextran1502 alextran1502 merged commit 038e69f into immich-app:main Feb 24, 2024
24 checks passed
@Jan108 Jan108 deleted the feat/password-toggle branch February 25, 2024 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants