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.

Sign up for GitHub

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

Jump to bottom

Improve password confirmation dialog #605

Merged
merged 1 commit into from
Oct 9, 2023
Merged

Improve password confirmation dialog #605

merged 1 commit into from
Oct 9, 2023

Conversation

JuliaKirschenheuter
Copy link
Contributor

@JuliaKirschenheuter JuliaKirschenheuter commented Oct 5, 2023 •
edited
Loading

@JuliaKirschenheuter JuliaKirschenheuter added the 2. developing Work in progress label Oct 5, 2023
@JuliaKirschenheuter JuliaKirschenheuter self-assigned this Oct 5, 2023
@JuliaKirschenheuter JuliaKirschenheuter mentioned this pull request Oct 5, 2023
Merged
4 tasks
@JuliaKirschenheuter JuliaKirschenheuter changed the title Upgrade @nextcloud/vue lib and improve password confirmation dialog Improve password confirmation dialog Oct 5, 2023
ShGKme
ShGKme requested changes Oct 5, 2023
View reviewed changes
src/components/PasswordDialog.vue Outdated Show resolved Hide resolved
@ShGKme
Copy link
Contributor

ShGKme commented Oct 5, 2023

Looks a little bit different than in the linked comment, but seems to be fine in terms of a11y

cc @nimishavijay

image

@ShGKme
Copy link
Contributor

ShGKme commented Oct 5, 2023

Upgrade to @nextcloud/vue@8 doesn't work without #606

@ShGKme ShGKme mentioned this pull request Oct 5, 2023
Merged
@JuliaKirschenheuter
Copy link
Contributor Author

Looks a little bit different than in the linked comment,

what look s different?

@ShGKme
Copy link
Contributor

ShGKme commented Oct 5, 2023

what look s different?

Label position

Nimisha's comment Current view
image image

@JuliaKirschenheuter
Copy link
Contributor Author

what look s different?

Label position

Recording.2023-09-27.182535.mp4

@JuliaKirschenheuter JuliaKirschenheuter force-pushed the fix/37080-update_nextcloud-vue_and_password_dialog branch from 101de3e to 5bc2765 Compare October 5, 2023 17:00
@JuliaKirschenheuter JuliaKirschenheuter marked this pull request as ready for review October 5, 2023 17:06
@ShGKme
Copy link
Contributor

ShGKme commented Oct 5, 2023

Is this problem in the comment from the linked issue also supposed to be fixed?

Should we do something with this message with failed authentication?

@ShGKme
Copy link
Contributor

ShGKme commented Oct 5, 2023

P.S. Don't forget to run

npm run l10n:extract

@JuliaKirschenheuter
Copy link
Contributor Author

@nickvergessen could i leave an error message like this

nextcloud-password-confirmation/src/components/PasswordDialog.vue

Line 87 in 9c2a2b7

errorText: t('Failed to authenticate, your password was wrong'),
? There is only one input field with password confirmation so i think it is ok to have more specific message. Please see nextcloud/server#37080 (comment)

@ShGKme
Copy link
Contributor

ShGKme commented Oct 6, 2023

What about using NcPasswordField's error state instead of a large message?
https://nextcloud-vue-components.netlify.app/#/Components/NcFields?id=ncpasswordfield

image

Like on the Login page:

image

Then it would be:

  • clear that this message is about the entered password
  • more accessible because of aria-describedby in the field
  • no so changeable in the height size
. .
image image

@JuliaKirschenheuter JuliaKirschenheuter marked this pull request as draft October 9, 2023 09:22
@JuliaKirschenheuter JuliaKirschenheuter force-pushed the fix/37080-update_nextcloud-vue_and_password_dialog branch 2 times, most recently from 467bdeb to 1c6fca7 Compare October 9, 2023 15:49
@JuliaKirschenheuter JuliaKirschenheuter marked this pull request as ready for review October 9, 2023 15:50
@JuliaKirschenheuter
Improve password confirmation dialog
b8b339c 
Signed-off-by: julia.kirschenheuter <julia.kirschenheuter@nextcloud.com>
@JuliaKirschenheuter JuliaKirschenheuter force-pushed the fix/37080-update_nextcloud-vue_and_password_dialog branch from 606d7e7 to b8b339c Compare October 9, 2023 16:35
@ShGKme ShGKme requested review from Pytal, susnux and artonge and removed request for susnux, artonge and Pytal October 9, 2023 16:51
artonge
artonge approved these changes Oct 9, 2023
View reviewed changes
src/components/PasswordDialog.vue Show resolved Hide resolved
@JuliaKirschenheuter JuliaKirschenheuter merged commit 695dc89 into master Oct 9, 2023
5 checks passed
@JuliaKirschenheuter JuliaKirschenheuter deleted the fix/37080-update_nextcloud-vue_and_password_dialog branch October 9, 2023 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ShGKme ShGKme ShGKme approved these changes

@artonge artonge artonge approved these changes

@nickvergessen nickvergessen Awaiting requested review from nickvergessen

@Pytal Pytal Awaiting requested review from Pytal

@susnux susnux Awaiting requested review from susnux

Assignees

@JuliaKirschenheuter JuliaKirschenheuter

Labels
2. developing Work in progress
Projects
None yet
Milestone
No milestone
5 participants
@JuliaKirschenheuter @ShGKme @susnux @artonge @Pytal