-
Notifications
You must be signed in to change notification settings - Fork 19
Conversation
Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes. |
methods: { | ||
setComboBoxAriaLabel() { | ||
const comboBoxElement = this.$refs.select.$el.querySelector("div:first-child") | ||
comboBoxElement.setAttribute("aria-label", this.$gettext("Search for option")) |
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.
instead of removing it it could be a better approach to use it as a fallback label if no label is provided
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.
hm i looked up all places where oc-select is used in web and they all set a label (with aria-label) outside of the oc-select, i think it makes more sence to leave the aria-labeling to the implementor since oc-select doesn't include a label.
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.
@kulmann @paulcod3 if we plan to merge this it'd be a breaking change and I would suggest cherry-picking it and bringing it inside the v9.0.0
release this week, too 🍒
@kulmann what's the way to go here? We could either drop the label as Paul suggested or "bring it in" to satisfy owncloud/web#8857. From what I know there's only two occurances in |
I'd like to work towards owncloud/web#8857 to have a consistent component API for form-related elements. |
Okay so closing this in order to provide a solution that works towards owncloud/web#8857 |
Description
See owncloud/web#5394
Related Issue
Types of changes
Checklist: