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

fix: improved accessibility of dropdown #954

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

Conversation

ypatadia-eightfold
Copy link
Collaborator

@ypatadia-eightfold ypatadia-eightfold commented Feb 26, 2025

SUMMARY:

change button to div (like radix UI)
tab to focus dropdown
enter to open dropdown
up/down/left/right key to navigate items inside dropdown
enter to select focused item of dropdown
esc/tab to close dropdown

GITHUB ISSUE (Open Source Contributors)

JIRA TASK (Eightfold Employees Only):

https://eightfoldai.atlassian.net/browse/ENG-132612

CHANGE TYPE:

  • Bugfix Pull Request
  • Feature Pull Request

TEST COVERAGE:

  • Tests for this change already exist
  • I have added unittests for this change

TEST PLAN:

Copy link

codesandbox-ci bot commented Feb 26, 2025

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Copy link

codecov bot commented Feb 26, 2025

Codecov Report

Attention: Patch coverage is 85.26316% with 14 lines in your changes missing coverage. Please review.

Project coverage is 84.45%. Comparing base (95290f7) to head (09b4417).

Files with missing lines Patch % Lines
src/components/Dropdown/Dropdown.tsx 84.37% 5 Missing ⚠️
src/components/List/List.tsx 68.75% 5 Missing ⚠️
src/shared/utilities/interactiveElements.ts 91.11% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #954      +/-   ##
==========================================
+ Coverage   84.44%   84.45%   +0.01%     
==========================================
  Files        1103     1104       +1     
  Lines       20557    20643      +86     
  Branches     7779     7817      +38     
==========================================
+ Hits        17359    17434      +75     
- Misses       3110     3121      +11     
  Partials       88       88              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ypatadia-eightfold ypatadia-eightfold force-pushed the ypatadia/ENG-132612/dropdown-accessibility-changes branch from 786522a to 8fc8a62 Compare March 6, 2025 19:59
};
// If clicking on an element inside a list item and closeOnElementClick is false,
// don't close the dropdown
if (!closeOnElementClick && isElementInsideListItem(event.target)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

you might need to check multiple as well.. If you have multi selection, then the close on element click might not help

@ypatadia-eightfold ypatadia-eightfold force-pushed the ypatadia/ENG-132612/dropdown-accessibility-changes branch from 16a7f3d to 835cd10 Compare March 17, 2025 15:27
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.

2 participants