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

feat(select accessibility): add <SingleSelectA11y/> component #1620

Open
wants to merge 39 commits into
base: master
Choose a base branch
from

Conversation

Mohammer5
Copy link
Contributor

@Mohammer5 Mohammer5 commented Oct 16, 2024

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 combobox points to the listbox via aria-controls={listBoxId} and aria-haspopup="listbox"
  • The menu is [role="listbox"]
    • The listbox notifies the user about changes via aria-live and aria-busy
  • Options are button elements with [role="option"]
  • Options have aria-selected, aria-disabled & aria-label
  • The search input has a aria-label when a filterLabel has 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 name prop.
    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 a component prop, which will override the default option style while the accessible option wrapper will stay intact
  • accepts a selected prop 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 clearText prop to be present when the select is clearable.
    This way we can have an aria-label on 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-test properties

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 A11y in 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 autoFocus prop, which was called initialFocus before. I didn't really think about the name until I realized it deviates. I suppose I should just use the old name?
    autofocus is the name of the actual HTML attribute
  • When using custom options, every option in the array has to have a component property. Would it make sense to introduce a optionComponent prop on the select which would apply to all options unless an individual option has a component property?
  • The listbox uses aria-live and aria-busy to update the user about changes. The aria-live attribute can have different values: off, polite & assertive. I think assertive could 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

  • Keyboard interactions
  • Loader at the bottom of the list is missing
  • Handle disabled states 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

  • API docs are generated
  • Types added / updated
  • Tests were added
  • Storybook demos were added

Code explanations

Component implementation:

SingleSelectA11y.-.Function.code.mp4

Test implementation:

SingleSelectA11y.-.Testing.mp4

Documentation:

SingleSelectA11y.-.Documentation.mp4

@dhis2-bot
Copy link
Contributor

dhis2-bot commented Oct 16, 2024

🚀 Deployed on https://pr-1620--dhis2-ui.netlify.app

@dhis2-bot dhis2-bot temporarily deployed to netlify October 16, 2024 02:48 Inactive
@Mohammer5 Mohammer5 requested a review from a team October 16, 2024 06:33
@Topener
Copy link
Contributor

Topener commented Oct 16, 2024

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.

@Mohammer5
Copy link
Contributor Author

@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.

@dhis2-bot dhis2-bot temporarily deployed to netlify October 17, 2024 10:51 Inactive
@dhis2-bot dhis2-bot temporarily deployed to netlify October 17, 2024 11:16 Inactive
@Topener
Copy link
Contributor

Topener commented Oct 17, 2024

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.

@dhis2-bot dhis2-bot temporarily deployed to netlify October 18, 2024 02:27 Inactive
@dhis2-bot dhis2-bot temporarily deployed to netlify October 21, 2024 10:45 Inactive
@dhis2-bot dhis2-bot temporarily deployed to netlify October 21, 2024 10:48 Inactive
@Mohammer5 Mohammer5 force-pushed the select-a11y branch 3 times, most recently from 1e0a40e to 25b4a65 Compare October 22, 2024 09:17
@dhis2-bot dhis2-bot temporarily deployed to netlify October 22, 2024 09:22 Inactive
@dhis2-bot dhis2-bot temporarily deployed to netlify October 22, 2024 09:32 Inactive
@Birkbjo Birkbjo self-requested a review October 22, 2024 09:34
@dhis2-bot dhis2-bot temporarily deployed to netlify October 23, 2024 00:43 Inactive
@dhis2-bot dhis2-bot temporarily deployed to netlify October 23, 2024 05:12 Inactive
@dhis2-bot dhis2-bot temporarily deployed to netlify October 23, 2024 08:49 Inactive
@dhis2-bot dhis2-bot temporarily deployed to netlify October 28, 2024 03:26 Inactive
@dhis2-bot dhis2-bot temporarily deployed to netlify October 28, 2024 07:06 Inactive
@dhis2-bot dhis2-bot temporarily deployed to netlify October 28, 2024 07:33 Inactive
@dhis2-bot dhis2-bot temporarily deployed to netlify October 28, 2024 09:50 Inactive
@dhis2-bot dhis2-bot temporarily deployed to netlify November 7, 2024 01:21 Inactive
@dhis2-bot dhis2-bot temporarily deployed to netlify November 7, 2024 01:33 Inactive
@dhis2-bot dhis2-bot temporarily deployed to netlify November 10, 2024 05:53 Inactive
@dhis2-bot dhis2-bot temporarily deployed to netlify November 10, 2024 06:52 Inactive
Copy link

sonarcloud bot commented Nov 28, 2024

Quality Gate Failed 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

@dhis2-bot dhis2-bot temporarily deployed to netlify November 28, 2024 08:58 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants