-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
base: main
Are you sure you want to change the base?
accessibity: make drodpowns accessible #9136
Conversation
Welcome!
Hello there, congrats on your first PR! We're excited to have you contributing to this project. |
There was a problem hiding this 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"
inRecordTableCellEditMode.tsx
- Incomplete ARIA implementation in
MenuItemSelect.tsx
- needsaria-selected
and proper parentrole="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
packages/twenty-front/src/modules/ui/layout/dropdown/components/Dropdown.tsx
Outdated
Show resolved
Hide resolved
packages/twenty-front/src/modules/ui/layout/dropdown/components/Dropdown.tsx
Outdated
Show resolved
Hide resolved
@@ -39,6 +39,7 @@ export const RecordTableCellEditMode = ({ | |||
<StyledEditableCellEditModeContainer | |||
ref={refs.setReference} | |||
data-testid="editable-cell-edit-mode-container" | |||
role="combobox" |
There was a problem hiding this comment.
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.
packages/twenty-ui/src/navigation/menu-item/components/MenuItemSelectAvatar.tsx
Show resolved
Hide resolved
packages/twenty-ui/src/navigation/menu-item/components/MenuItemSelectAvatar.tsx
Show resolved
Hide resolved
@@ -67,6 +67,7 @@ export const MenuItemSelect = ({ | |||
selected={selected} | |||
disabled={disabled} | |||
hovered={hovered} | |||
role="option" |
There was a problem hiding this comment.
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.
No description provided.