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(Chip): accessibility improvements #1471

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

Conversation

alaa-yahia
Copy link
Member

@alaa-yahia alaa-yahia commented Apr 8, 2024

Implements LIBS-552


Description

Implement:
LIBS-553
LIBS-554
LIBS-555

  • Add ChipGroup component.
  • Update Chip component to accept component prop.
  • ChipGroup acts as a composite component, a user can tab into ChipGroup.
  • When tabbing into ChipGroup the Chip with selected gets focus.
  • If there isn't any Chip with selected, the focus goes to the first Chip
  • A user can use left/right arrow keys to move between Chip components.
  • Pressing the left/right arrow keys will cycle back to the first or last Chip, ensuring continuous navigation.
  • ChipGroup has role listbox
  • Chip has role option.

Checklist

  • API docs are generated
  • Tests were added
  • Storybook demos were added

@alaa-yahia alaa-yahia requested a review from a team as a code owner April 8, 2024 03:13
@dhis2-bot
Copy link
Contributor

dhis2-bot commented Apr 8, 2024

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

@dhis2-bot dhis2-bot temporarily deployed to netlify April 8, 2024 03:17 Inactive
Copy link

cypress bot commented Apr 8, 2024

Passing run #3424 ↗︎

0 584 0 0 Flakiness 0

Details:

fix: avoid using refs
Project: ui Commit: 27b0671575
Status: Passed Duration: 06:25 💡
Started: Jul 16, 2024 10:26 AM Ended: Jul 16, 2024 10:32 AM

Review all test suite changes for PR #1471 ↗︎

}

return (
<button
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Chips are mainly used for links. Button is definitely better than the previous span, but I think we should look into how we would support links, instead of having to do the navigation imperatively in onClick handler. It's not valid HTML to wrap a button in a link, so this component would have to support it.

It might be worth supporting a component-prop, and href as props. See how material UI does it: https://mui.com/material-ui/react-chip/#clickable-link . A lot of times you would want to use a Chip with eg. react-routers NavLink.

@kabaros @ismay

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it sounds correct .. but not in the scope of this PR, right? we can add a separate ticket for it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, but we may want to release that together. I brought it up because this PR changes the underlying element from span to a button. And wrapping a button in anchor tag is not valid, so it's potentially breaking .

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a breaking change for sure.
Two things:
Links should not have the role = option.
The accessibility features require that each chip be a direct child of the chipGroup.

I can change the button back to span and handle the enter key event for now or add a component, to & href props which in this case will pass the role=option to links.

if you have another way of doing it suggest it please. @Birkbjo @kabaros

@dhis2-bot dhis2-bot temporarily deployed to netlify April 17, 2024 21:19 Inactive
Copy link
Collaborator

@kabaros kabaros left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very good PR, not an easy one to implement - two things missing for me:

  • I am able to navigate through the chips, but I don't get a visual feedback that a chip is selected. They should have a focused style similar to buttons. (see screenshot below)

  • Please also add a StoryBook to show the new component ChipGroup, and update the documentation page (for Chip, or separate one) to mention this new component and its use.

chip-accessibility

if (!onRemove) {
return
}
if (event.key === 'Backspace' || event.key === 'Delete') {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say also event.stopPropagation here

}

return (
<button
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it sounds correct .. but not in the scope of this PR, right? we can add a separate ticket for it

@alaa-yahia alaa-yahia marked this pull request as draft July 3, 2024 10:15
@dhis2-bot dhis2-bot temporarily deployed to netlify September 2, 2024 18:26 Inactive
@dhis2-bot dhis2-bot temporarily deployed to netlify September 2, 2024 20:36 Inactive
@dhis2-bot dhis2-bot temporarily deployed to netlify September 2, 2024 20:47 Inactive
@alaa-yahia alaa-yahia marked this pull request as ready for review September 2, 2024 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants