[WC-3015] Improve dropdown filter #1732
Conversation
packages/shared/widget-plugin-dropdown-filter/src/controls/select/Select.tsx
Show resolved
Hide resolved
r0b1n
force-pushed
the
improve-dropdown-filter
branch
3 times, most recently
from
5ceb469 to
0cd24af
Compare
r0b1n
changed the title
r0b1n
force-pushed
the
improve-dropdown-filter
branch
2 times, most recently
from
4b3d234 to
d92da8a
Compare
| <button className={cls.toggle} {...getToggleButtonProps({ "aria-label": "Show options" })}> | ||
| <Arrow className={cls.stateIcon} /> | ||
| </button> | ||
| <OptionsWrapper | ||
| cls={cls} | ||
| label={inputLabel} |
There was a problem hiding this comment.
Why do we set the input label on the Options menu? This seems very counterintuitive.
There was a problem hiding this comment.
This is the logic behind downshiftjs, it was putting aria-labeledby on menu as well. And because we didn't render label it was a dangling property, pointing to a non-existing element by id. So I replaced it with explicit aria-label, which should have the same effect downshiftjs intended to have. Check this line to see how snapshot changed.
Other option is to explicitly unset this aria-labeledby that is coming from getMenuProps, so it won't have a label, but I thought that downshiftjs has a good reason to label menu as well.
There was a problem hiding this comment.
Yes, since we're using it here without the label provided by downshift I would also say this makes more sense. We could also consider having the label from downshift there but invisible? But to me this aria-label approach makes more sense. Let me know what you think @iobuhov
| @@ -142,13 +146,31 @@ | |||
| </property> | |||
| </propertyGroup> | |||
| </propertyGroup> | |||
| <propertyGroup caption="Accessibility"> | |||
| <propertyGroup caption="Advanced"> | |||
| <propertyGroup caption="Accessibility"> | |||
| <property key="ariaLabel" type="textTemplate" required="false"> | |||
| <caption>Input caption</caption> | |||
| <description>Assistive technology will read this upon reaching the input element.</description> | |||
There was a problem hiding this comment.
I think this description is not relevant anymore. In case of simple select, aria label is added to the menu, which is confusing. Should we change description?
There was a problem hiding this comment.
This is probably still holds, as if this says something like "Select color", it will be read upon reaching the filter widget and also while user selects options from the list, and the list is also labeled "Select color", which is I think a11y implementation details not worth mentioning here.
| showSeparator={false} | ||
| visible={!props.empty} | ||
| /> | ||
| <ClearButton cls={cls} onClick={props.onClear} visible={!props.empty} /> |
There was a problem hiding this comment.
Just tested the combobox and found that after clearing selection, keyboard navigation breaks because focus is not returned to the trigger element. Focus is rests to body
There was a problem hiding this comment.
Good point, thanks! Pushed a fix just now.
separator line margin between tags
also use clear button again
this is how combobox works
…button up the html tree - makes it easier to open the popup, previously it was not opening if clicked inside, but next to the border. - fixes issue with nested button tags.
- introduce new options to configure placeholder text and empty caption - show placeholder when widget is not open and empty - split emptySelectionCaption and emptyOptionCaption
r0b1n
force-pushed
the
improve-dropdown-filter
branch
from
d92da8a to
7b63b01
Compare
Pull request type
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Description
Various improvements for dropdown widget.
Breaking change
The input text are now changed and it will affect users, they have to revisit and configure text to their needs.