-
Notifications
You must be signed in to change notification settings - Fork 173
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
2565: Scalable keyboard accessibility for Dropdown in User Settings Dialog #2691
Conversation
…into 2565-language-dropdown
Please make sure the selector of the dropdown can be applied to any dropdown independently from the language, index, aria-label, placeholder, etc. Most likely the unique class or id added to the HTML might help. Keeping in mind there might be different content inside of the dropdown we call "language dropdown" and there could be multiple dropdowns on the same view. |
Couple of things to adjust from my side.
Thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
core/src/UserSettingsEditor.svelte
Outdated
list.item(chosenElementIndex).classList.remove('is-selected'); | ||
list.item(chosenElementIndex).classList.remove('is-focus'); | ||
chosenElementIndex += 1; | ||
list.item(chosenElementIndex).classList.add('is-selected'); | ||
list.item(chosenElementIndex).classList.add('is-focus'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these lines repeated quite often, could you optimize it and remove console.log?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Great job!
Changes proposed in this pull request:
Related issue(s)
#2565