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

accessibity: make drodpowns accessible #9136

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

malomarrec
Copy link

No description provided.

Copy link

github-actions bot commented Dec 18, 2024

Welcome!

Hello there, congrats on your first PR! We're excited to have you contributing to this project.
By submitting your Pull Request, you acknowledge that you agree with the terms of our Contributor License Agreement.

Generated by 🚫 dangerJS against c4e6802

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 PR adds initial accessibility improvements to dropdown components by introducing ARIA attributes and roles, though several implementations are incomplete or need refinement.

  • Hardcoded ID "currency-picker-options" in Dropdown.tsx creates potential conflicts with multiple instances
  • Empty fragment in CurrencyPickerDropdownButton.tsx breaks currency selection functionality
  • Missing required ARIA attributes for role="combobox" in RecordTableCellEditMode.tsx
  • Incomplete ARIA implementation in MenuItemSelect.tsx - needs aria-selected and proper parent role="listbox"
  • Missing keyboard navigation handlers across dropdown components for proper accessibility compliance

💡 (1/5) You can manually trigger the bot by mentioning @greptileai in a comment!

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

@@ -39,6 +39,7 @@ export const RecordTableCellEditMode = ({
<StyledEditableCellEditModeContainer
ref={refs.setReference}
data-testid="editable-cell-edit-mode-container"
role="combobox"
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: combobox role requires aria-expanded, aria-controls, and aria-haspopup attributes. Also needs aria-activedescendant when expanded.

@@ -67,6 +67,7 @@ export const MenuItemSelect = ({
selected={selected}
disabled={disabled}
hovered={hovered}
role="option"
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: role='option' requires a parent element with role='listbox' to be semantically valid. Also missing required aria-selected attribute to match the selected prop.

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.

1 participant