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(dropdown.tsx): Dropdown Accessibility #840

Merged
merged 11 commits into from
Jul 6, 2023

Conversation

nigellima
Copy link
Collaborator

@nigellima nigellima commented Jul 4, 2023

Description

This PR enables the Dropdown.tsx component to be navigated by keyboard, improves the Dropdown.Item component to be rendered using as prop, and fixes the dismiss on click and disabled trigger issues.

Fixes #648. Fixes #793 . Fixes #539

dropdown-a11y

The package resolution added in package.json was necessary due to some errors being thrown in the tests when using FloatingFocusManager component. See: focus-trap/focus-trap-react#986

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • This change contains documentation update - PENDING

How Has This Been Tested?

TBD

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • I have checked my code and corrected any misspellings

@vercel
Copy link

vercel bot commented Jul 4, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
flowbite-react ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 6, 2023 3:34pm

@nigellima nigellima marked this pull request as draft July 4, 2023 03:55
@nigellima nigellima changed the title feat(dropdow.tsx): Dropdown Accessibility feat(dropdown.tsx): Dropdown Accessibility Jul 4, 2023
This component is responsible to handle the logic of creating the button element based on the custom or default tag name
Now the Dropdown component is accessible by keyboard. Also the dismiss on item click is fixed

fix themesberg#648. fix themesberg#793
clone element resulted from renderTrigger and attach the ref and a11y props to it
Resolution for dependency nwsapi was added in order to fix an issue in the tababble package
@codecov
Copy link

codecov bot commented Jul 6, 2023

Codecov Report

Patch coverage: 99.73% and project coverage change: -0.02 ⚠️

Comparison is base (7461173) 99.54% compared to head (6f4418b) 99.53%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #840      +/-   ##
==========================================
- Coverage   99.54%   99.53%   -0.02%     
==========================================
  Files         163      165       +2     
  Lines        6621     6823     +202     
  Branches      401      414      +13     
==========================================
+ Hits         6591     6791     +200     
- Misses         30       32       +2     
Impacted Files Coverage Δ
src/components/Dropdown/Dropdown.tsx 99.22% <99.41%> (-0.78%) ⬇️
src/components/Button/Button.tsx 100.00% <100.00%> (ø)
src/components/Button/ButtonBase.tsx 100.00% <100.00%> (ø)
src/components/Button/theme.ts 100.00% <100.00%> (ø)
src/components/Carousel/Carousel.tsx 99.02% <100.00%> (+0.06%) ⬆️
src/components/DarkThemeToggle/DarkThemeToggle.tsx 100.00% <100.00%> (ø)
src/components/Dropdown/DropdownItem.tsx 100.00% <100.00%> (ø)
src/components/Dropdown/theme.ts 100.00% <100.00%> (ø)
src/components/Floating/Floating.tsx 100.00% <100.00%> (ø)
src/components/Modal/theme.ts 100.00% <100.00%> (ø)
... and 6 more

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@nigellima nigellima marked this pull request as ready for review July 6, 2023 06:15
Copy link
Collaborator

@rluders rluders left a comment

Choose a reason for hiding this comment

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

Wow! Really nice, @nigellima. I added just a few comments, but in general, it looks great.

src/helpers/floating.ts Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
src/components/Dropdown/DropdownItem.tsx Outdated Show resolved Hide resolved
src/components/Dropdown/DropdownItem.tsx Show resolved Hide resolved
theme.floating.base,
theme.floating.animation,
'duration-100',
!open && theme.floating.hidden,

Check warning

Code scanning / CodeQL

Useless conditional Warning

This negation always evaluates to false.
Copy link
Collaborator

@rluders rluders left a comment

Choose a reason for hiding this comment

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

Thank you, @nigellima.

@rluders rluders merged commit 65b13e7 into themesberg:main Jul 6, 2023
6 checks passed
@tulup-conner
Copy link
Collaborator

This is another fantastic PR, thank you very much!

@maxlibin
Copy link

maxlibin commented Jul 26, 2023

In light of the changes made in this link: https://github.com/themesberg/flowbite-react/pull/840/files#diff-32fa9ddaf531f010c646834b9023bf97df1cd80d6de82762a56bc99514594f83R238, it appears that everything within the dropdown is now contained in an <ul> tag. Previously, I could incorporate a form complete with input fields inside the dropdown; however, that functionality no longer seems operational. Is this an intentional modification?

Additionally, I found some documentation here: https://github.com/themesberg/flowbite-react/pull/840/files#diff-d5c7e7428127c9af056fda02409bfef933507c13fa5dad102ee8dc33415b3c55R94. It suggests that I can use as="span". However, this suggestion raises a question for me: Are we now inserting a span element within the ul? To me, this implementation feels unusual.

Please provide clarification if my understanding of these changes is mistaken. I appreciate your guidance 🙏.

@rluders
Copy link
Collaborator

rluders commented Jul 26, 2023

The Dropdown children will be always a Dropdown.Item, the component that you can "morph" is the Trigger, which is outside the ul and the Dropdown.Item's button that is inside the li. So... Unless I'm not seeing something here, I don't see any issue with this implementation. And yes, the ul > li structure was desired.

@maxlibin
Copy link

thanks for clarification

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants