feat(select accessibility): add <SingleSelectA11y/> component
Implements LIBS-559
Storybook demos here.
Description
Adds an accessible version of the single select component. This component is significantly different from the existing single select component in some aspects, so I decided to create a new component rather than modifying the old one.
In order to make this component accessible:
- The selected value is
[role="combobox"]- The combobox has
aria-placeholder,aria-labelledby&aria-expanded - The arrow inside the combobox is a button
- Clicking the button will focus the combobox
- The
comboboxpoints to thelistboxviaaria-controls={listBoxId}andaria-haspopup="listbox"
- The combobox has
- The menu is
[role="listbox"]- The listbox notifies the user about changes via
aria-liveandaria-busy
- The listbox notifies the user about changes via
- Options are
buttonelements with[role="option"] - Options have
aria-selected,aria-disabled&aria-label - The search input has a
aria-labelwhen afilterLabelhas been provided
Style-wise there should be no difference between the existing component and the new one. There are some differences in which props exist and which props are expected:
The new component
- expects a
nameprop.
The HTML element that displays the selected value (combobox) needs to point to the element that has the options (listbox), which is done by using id attributes. Instead of creating one randomly, I decided to make this prop requried. - expects an options array rather than children.
This makes it easier to access the options without having to go through children. In order to support custom options, the options can have acomponentprop, which will override the default option style while the accessible option wrapper will stay intact - accepts a
selectedprop which is an option ({ value: string, label: string }).
This should help providing the selected value and its label while still loading the options. This eliminates an issue we've had in the past with other components where the selected option would have to be added to the options array, regardless of whether it should be displayed or not - requires the
clearTextprop to be present when the select isclearable.
This way we can have anaria-labelon the clear button. - has a filter, which is now controlled by the consumer (
filterValue&onFilterChange).
Consumers can now use the filter to load more options or control the options list in any other way - has a different set of
data-testproperties
Breaking changes (disregarding prop changes) if we'd replace the existing component
- The component can't be given an arbitrary set of children. It doesn't accept any children whatsoever
- Filtering doesn't work out of the box
Thinks I want to discuss
- [ ] The new component has
A11yin its name. I don't think we should replace the old component but deprecate it to give developers time to transition to the accessible variants. But how should we call the new one? - [ ] The new component accepts an
autoFocusprop, which was calledinitialFocusbefore. I didn't really think about the name until I realized it deviates. I suppose I should just use the old name?
autofocusis the name of the actual HTML attribute - [x] When using custom options, every option in the array has to have a
componentproperty. Would it make sense to introduce aoptionComponentprop on the select which would apply to all options unless an individual option has acomponentproperty? - [ ] The listbox uses
aria-liveandaria-busyto update the user about changes. Thearia-liveattribute can have different values:off,polite&assertive. I thinkassertivecould be better, but depends on the use-case I suppose. Do we want the consumer to be able to control this value? And if yes, what should the default value be? - [ ] The transfer component has areas in which the app can place anything.
I think it could be useful for the transfer to do the same (above and below the options (and the listbox HTML element) in the menu). We could even get rid of the search functionality here and let apps handle that, making the component leaner and reducing the maintenance burden in the app.
Open TODOs
- [x] Keyboard interactions
- [x] Loader at the bottom of the list is missing
- [x] Handle
disabledstates when handling keyboard inputs - [ ] Padding needs to be added to the filter field
- The commented out padding causes the intersection observer to not detect the last option (when adding the styles back) as having become visible when scrolling down. The issue is described in this stackoverflow article, although in our case it's not about a child but about a sibling element. I already tried using an inner div and assign the padding to that element, but that caused the same problem...
- I think this should not prevent us from merging the PR, as this issue doesn't impede functionality
Checklist
- [x] API docs are generated
- [ ] Types added / updated
- [x] Tests were added
- [x] Storybook demos were added
Code explanations
Component implementation:
https://github.com/user-attachments/assets/628c5f53-f55c-4fec-a7a4-a7550dafff4e
Test implementation:
https://github.com/user-attachments/assets/bd1ec320-da01-4343-910c-0f647b75bc4a
Documentation:
https://github.com/user-attachments/assets/483e9f4e-6ead-4ccb-bd4b-1ba1e3c8aba3
🚀 Deployed on https://pr-1620--dhis2-ui.netlify.app
What exactly are the breaking changes compared to the previous input besides new props and features? Are certain functionalities no longer working?
I would say a breaking change release would be better than introducing a new component, and if both components should be kept in the package maybe the old one should be renamed so people have the option to upgrade with just a name change?
I am also missing some documentation about this in the docs/docs directory.
@Topener
What exactly are the breaking changes compared to the previous input besides new props and features? Are certain functionalities no longer working?
The existing component accepts children, which means you can pass anything to the component and it will be rendered in the menu, which you can see here. This means that apps that rely on this feature can't do the same with the new component.
Other potential breaking changes relate to custom options: The options in the new component uses a button element and customization can only be done within the button. If apps were putting block elements or several buttons into a single option, this wouldn't be possible anymore.
I would say a breaking change release would be better than introducing a new component, and if both components should be kept in the package maybe the old one should be renamed so people have the option to upgrade with just a name change?
I can elaborate my reasoning for introducing an additional component with a new name: I'm mostly worried about the impact on devs when they want to upgrade UI for other reasons. Say we introduce a breaking change but the teams don't have time at that moment to transition, then release another feature that is unrelated to the select component and one of the apps needs that change. If the app has many selects, it'd be a lot of work. Releasing two components, deprecating the old one and keeping its name for the time-being would allow devs a transition time.
Of course we could force apps to use the new component and be more accessible asap. This mostly depends on how we want to move forward organization-wide. My stance is to be careful which breaking changes in essential components and give developers enough time to transition from one component to another without stopping them from upgrading the library when they need other changes.
I am also missing some documentation about this in the docs/docs directory.
Yes, this is still draft and the docs checkbox isn't checked. The goal is to discuss the general direction now that I have something I can show.
Alright I get the reasoning! I'm not the one to decide that but that would be my recommendation. Releasing a new major version also allows for breaking changes and I think that is the best way forward as it would be a new best practice. It also allows people to stay on an older version if they don't have time to upgrade. We'll just have to document the breaking change properly, not only in the releasenotes but also in the /docs/docs directory with the component
Perhaps @amcgee can share his opinion on the matter.
Quality Gate failed
Failed conditions
44 New issues
2 New Bugs (required ≤ 0)
5 New Critical Issues (required ≤ 0)
B Reliability Rating on New Code (required ≥ A)
42 New Code Smells (required ≤ 0)
See analysis details on SonarQube Cloud
Catch issues before they fail your Quality Gate with our IDE extension
SonarQube for IDE
Quality Gate failed
Failed conditions
48 New issues
B Reliability Rating on New Code (required ≥ A)
5 New Critical Issues (required ≤ 0)
46 New Code Smells (required ≤ 0)
2 New Bugs (required ≤ 0)
See analysis details on SonarQube Cloud
Catch issues before they fail your Quality Gate with our IDE extension
SonarQube for IDE
thanks @Mohammer5 for the great work here
:tada: This PR is included in version 10.10.0 :tada:
The release is available on:
- npm package (@latest dist-tag)
- npm package (@latest dist-tag)
- npm package (@latest dist-tag)
- npm package (@latest dist-tag)
- npm package (@latest dist-tag)
- npm package (@latest dist-tag)
- npm package (@latest dist-tag)
- npm package (@latest dist-tag)
- npm package (@latest dist-tag)
- npm package (@latest dist-tag)
- npm package (@latest dist-tag)
- npm package (@latest dist-tag)
- npm package (@latest dist-tag)
- npm package (@latest dist-tag)
- npm package (@latest dist-tag)
- npm package (@latest dist-tag)
- npm package (@latest dist-tag)
- npm package (@latest dist-tag)
- npm package (@latest dist-tag)
- npm package (@latest dist-tag)
- npm package (@latest dist-tag)
- npm package (@latest dist-tag)
- npm package (@latest dist-tag)
- npm package (@latest dist-tag)
- npm package (@latest dist-tag)
- npm package (@latest dist-tag)
- npm package (@latest dist-tag)
- npm package (@latest dist-tag)
- npm package (@latest dist-tag)
- npm package (@latest dist-tag)
- npm package (@latest dist-tag)
- npm package (@latest dist-tag)
- npm package (@latest dist-tag)
- npm package (@latest dist-tag)
- npm package (@latest dist-tag)
- npm package (@latest dist-tag)
- npm package (@latest dist-tag)
- npm package (@latest dist-tag)
- npm package (@latest dist-tag)
- npm package (@latest dist-tag)
- npm package (@latest dist-tag)
- npm package (@latest dist-tag)
- npm package (@latest dist-tag)
- npm package (@latest dist-tag)
- npm package (@latest dist-tag)
- npm package (@latest dist-tag)
- npm package (@latest dist-tag)
- npm package (@latest dist-tag)
- npm package (@latest dist-tag)
- npm package (@latest dist-tag)
- GitHub release
Your semantic-release bot :package::rocket: