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

fix(settings): focus lost on selects in users settings #39909

Merged
merged 4 commits into from
Aug 16, 2023

Conversation

ShGKme
Copy link
Contributor

@ShGKme ShGKme commented Aug 16, 2023

Summary

After adding/removing groups the focus is lost from NcSelect. You need to catch the select again with tab. This happens because during loading the NcSelect become disabled and cannot handle focus. To fix - remove disabling on loading state.

Also fixed some very minor issues in this component.

🖼️ Screenshots

Before After
before after
image image
image image

Todo

  • Don't disable NcSelects on loading, only keep its loading state
  • Minor fixes
    • Fix loading indicator of user manager update (lost await)
    • Fix label set user manager label

Drawbacks

Now NcSelect is still available during the loading, so the user can add a new group or delete a group. But it should work fine and it is the same as what we have with collaborative tags in the Files sidebar.

Checklist

@JuliaKirschenheuter
Copy link
Contributor

JuliaKirschenheuter commented Aug 16, 2023

should we remove

groups: false,
subadmins: false,
quota: false,
delete: false,
disable: false,
languages: false,
wipe: false,
manager: false,

and all other related code for this.idState.xxx
this.idState.loading.groups = true
too?

Copy link
Contributor

@JuliaKirschenheuter JuliaKirschenheuter left a comment

Choose a reason for hiding this comment

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

Thank you!

I guess a text here doesn't have enough contrast ;(
image
Is it an issue of NcSelect?

@ShGKme
Copy link
Contributor Author

ShGKme commented Aug 16, 2023

should we remove

It is still used for loading indicators, I only remove its usage from :disable

@ShGKme
Copy link
Contributor Author

ShGKme commented Aug 16, 2023

Is it an issue of NcSelect?

Yes...

@Pytal Pytal added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Aug 16, 2023
@Pytal Pytal merged commit db3e60c into master Aug 16, 2023
40 checks passed
@Pytal Pytal deleted the fix/a11y-users-list-select-focus-on-tag-delete branch August 16, 2023 22:22
@welcome
Copy link

welcome bot commented Aug 16, 2023

Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22

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 accessibility bug feature: users and groups
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants