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

Replace input filed with password field and added password error message #42030

Merged
merged 1 commit into from
Dec 8, 2023

Conversation

JuliaKirschenheuter
Copy link
Contributor

@JuliaKirschenheuter JuliaKirschenheuter commented Dec 5, 2023

Summary

  • Replace input filed with password field and added password error message
  • Remove unneeded NcDateTimePicker

Before:

Screenshot from 2023-12-05 11-12-07

After:

Peek 2023-12-05 10-47

Checklist

@JuliaKirschenheuter JuliaKirschenheuter added the 3. to review Waiting for reviews label Dec 5, 2023
@JuliaKirschenheuter JuliaKirschenheuter self-assigned this Dec 5, 2023
@JuliaKirschenheuter
Copy link
Contributor Author

/backport to stable28

Copy link
Contributor

@ShGKme ShGKme left a comment

Choose a reason for hiding this comment

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

Fixes the issue, but has 2 drawbacks:

  • Now a user always needs +1 more click to see the generated password
  • On edit of saved password, this might not be clear that an empty password field is a new password field and there is a password.

@nextcloud/designers What do you think?

@JuliaKirschenheuter JuliaKirschenheuter added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Dec 6, 2023
@jancborchardt
Copy link
Member

Now a user always needs +1 more click to see the generated password

This I’d say is less of an issue

On edit of saved password, this might not be clear that an empty password field is a new password field and there is a password.

@ShGKme I’m not sure I fully understand, especially the part "[…] is a new password field and there is a password" – it looks fine from your posted video above, but it only shows one flow.

@ShGKme
Copy link
Contributor

ShGKme commented Dec 6, 2023

This I’d say is less of an issue

Just to be clear, a user always needs it because checking "set password" generates a random password.

password

@ShGKme I’m not sure I fully understand, especially the part "[…] is a new password field and there is a password" – it looks fine from your posted video above, but it only shows one flow.

When you open an existing share with a password, you see:

image

I'm not sure that it is clear that it is "enter a new password to change it" and not "you have set a password and it is an empty password".

@jancborchardt
Copy link
Member

@ShGKme thanks for explaining. Well both of those cases are confusing, the second one even more than the first. In both cases, ideally the password should be shown in plain text from a UX point of view.

If that is a security concern, instead of the plain input field we could instead show feedback text like:

  • New: Password generated [ Copy ] [ Edit ]
  • Existing: Password set [ Copy ] [ Edit ]

That way it would also be a 2-step process, but way clearer what it is about, and clicking on "Edit" should also directly show the password in plain text.

@ShGKme
Copy link
Contributor

ShGKme commented Dec 6, 2023

In both cases, ideally the password should be shown in plain text from a UX point of view.

We can show the password only when a user checks "Set password" and a new password is generated.

Here the only issue could be that "Show password" is false by default in NcPasswordPicker.

But for security concerns, we don't actually store the password. So when users edit an existing entry, they can set a new password, but can neither see nor edit the previously entered password.

@JuliaKirschenheuter
Copy link
Contributor Author

Dear designers,

a kindly reminder: this is only a small a11y issue regarding error text which was not there before. We can do all improvements after certification 🙏. Do you have any considerations regarding this error text? If not, please approve this PR.

Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

Error text is fine, so not blocked from that side @JuliaKirschenheuter :)

Remove unneeded NcDateTimePicker

Signed-off-by: julia.kirschenheuter <[email protected]>
@JuliaKirschenheuter JuliaKirschenheuter force-pushed the fix/41882-add-password-message branch from 97b5f0a to 696545b Compare December 8, 2023 09:23
@JuliaKirschenheuter JuliaKirschenheuter merged commit b0a60de into master Dec 8, 2023
42 checks passed
@JuliaKirschenheuter JuliaKirschenheuter deleted the fix/41882-add-password-message branch December 8, 2023 10:20
@nfebe
Copy link
Contributor

nfebe commented Dec 9, 2023

We can do all improvements after certification 🙏. Do you have any considerations regarding this error text? If not, please approve this PR.

@JuliaKirschenheuter please, you should track a new issue for the improvements, so we ensure it gets done. Thank you...

@AndyScherzinger AndyScherzinger added 4. to release Ready to be released and/or waiting for tests to finish and removed 2. developing Work in progress labels Dec 15, 2023
@AndyScherzinger AndyScherzinger added this to the Nextcloud 29 milestone Dec 15, 2023
@blizzz blizzz mentioned this pull request Mar 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish backport-request
Projects
None yet
7 participants