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

Use 'role' = button for chip navigation #8011

Merged
merged 5 commits into from
Oct 24, 2024
Merged

Conversation

BKM14
Copy link
Contributor

@BKM14 BKM14 commented Oct 23, 2024

Closes #7817
Added role attribute to the div element of the Chip component. This assigns the role of "button" to the container, which is important for accessibility. It indicates that this div should be treated as a button by assistive technologies like screen readers.

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This pull request adds the 'role' attribute with the value 'button' to the Chip component's StyledContainer, improving accessibility for assistive technologies.

  • Added 'role: "button"' to StyledContainer's type definition in packages/twenty-ui/src/display/chip/components/Chip.tsx
  • Included 'role="button"' prop in the JSX of StyledContainer
  • Addresses accessibility concern raised in issue Use <a> or role=button for Chip Navigation #7817 for Chip navigation
  • Potential improvement: Consider using <a> element for navigation items as suggested in the issue
  • Note: Further accessibility enhancements may be needed, such as keyboard navigation support

1 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile

@@ -133,6 +133,7 @@ export const Chip = ({
size={size}
variant={variant}
onClick={onClick}
role="button"
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Consider conditionally applying role='button' only when onClick is provided

@Devessier
Copy link
Contributor

I'm not sure about the proposed change. I would like to examine the initial problem and consider another solution.

We should be using a <a> element for links. Setting an explicit HTML role shouldn't be automatic; we should usually rely on semantic HTML. When semantic HTML is insufficient, we can set an explicit role attribute but must ensure that the element behaves correctly. Here, we create a fake button. See this video to understand how complex native HTML button elements are: https://youtu.be/CZGqnp06DnI.

Let's keep this work on stand-by and think about another solution.

@FelixMalfait
Copy link
Member

Thanks!
Indeed it would be better to create proper links rather than add the role button.
I started a small refactoring to pass proper link but I didn't go as far as we should, there's still an issue on useOpenRecordTableCellV2 where we use navigate and not a proper Link, we can update that later.

At least now Thomas will be able to use Vimium

@FelixMalfait FelixMalfait merged commit 4e59f00 into twentyhq:main Oct 24, 2024
15 of 16 checks passed
@BKM14 BKM14 deleted the v1 branch October 24, 2024 14:05
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.

Use <a> or role=button for Chip Navigation
3 participants