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

Set visible label for input field #40643

Merged

Conversation

JuliaKirschenheuter
Copy link
Contributor

@JuliaKirschenheuter JuliaKirschenheuter commented Sep 26, 2023

Summary

Replace password dialog with dialog from nextcloud-password-confirmation library. No visual changes.

Checklist

@ShGKme
Copy link
Contributor

ShGKme commented Sep 26, 2023

Looks a bit weird... Would it be valid here to use "This action requires you to confirm your password" as a label?

@JuliaKirschenheuter
Copy link
Contributor Author

Looks a bit weird... Would it be valid here to use "This action requires you to confirm your password" as a label?

I think "This action requires you to confirm your password" is too long for a label. That would change width of modal window too.
Would you have some other suggestion? But actually we can do all improvements after a11y certification.

@JuliaKirschenheuter
Copy link
Contributor Author

@nimishavijay would you approve it?

@nimishavijay
Copy link
Member

nimishavijay commented Sep 27, 2023

Would using a password field with an internal label work?

Recording.2023-09-27.182535.mp4

If not, then I propose the following changes:

image

  • Change heading to "Confirm your password"
  • Change subline to "This action needs authentication"
  • Use an external label, but on the same row as the input field

Done here: nextcloud-libraries/nextcloud-password-confirmation#605

@ShGKme
Copy link
Contributor

ShGKme commented Oct 6, 2023

@artonge Do you want to re-review after a large change?

Copy link
Member

@Pytal Pytal left a comment

Choose a reason for hiding this comment

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

Still needs #40874

Otherwise good

@JuliaKirschenheuter JuliaKirschenheuter force-pushed the fix/37082-set_visible_label_for_input_in_modal branch 2 times, most recently from 657706b to 27f39c3 Compare October 16, 2023 11:49
Signed-off-by: julia.kirschenheuter <julia.kirschenheuter@nextcloud.com>
@JuliaKirschenheuter JuliaKirschenheuter force-pushed the fix/37082-set_visible_label_for_input_in_modal branch from 27f39c3 to 7d00c7f Compare October 17, 2023 12:10
@ShGKme
Copy link
Contributor

ShGKme commented Oct 17, 2023

Cypress users_modify seems to be a flaky test, works fine locally:
image

Another test is a known problem

@JuliaKirschenheuter
Copy link
Contributor Author

failing tests are unrelated

@JuliaKirschenheuter JuliaKirschenheuter merged commit fbf8a46 into master Oct 17, 2023
37 of 41 checks passed
@JuliaKirschenheuter JuliaKirschenheuter deleted the fix/37082-set_visible_label_for_input_in_modal branch October 17, 2023 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants