search

zakodium-oss / react-science

React components and tools to build scientific applications.
https://react-science.pages.dev
MIT License
2 stars 6 forks source link

Close select on escape key #645

Closed wadjih-bencheikh18 closed 6 months ago

wadjih-bencheikh18 commented 7 months ago

closes : https://github.com/zakodium-oss/react-science/issues/632

closes : https://github.com/zakodium-oss/react-science/issues/622

cloudflare-pages[bot] commented 7 months ago

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 0198dce
Status: ✅  Deploy successful!
Preview URL: https://10f61b34.react-science.pages.dev
Branch Preview URL: https://632-select-does-not-close-on.react-science.pages.dev

View logs

codecov-commenter commented 7 months ago

Codecov Report

Attention: 183 lines in your changes are missing coverage. Please review.

Comparison is base (6c34c34) 23.51% compared to head (0198dce) 22.13%. Report is 12 commits behind head on main.

Files Patch % Lines
stories/components/select.stories.tsx 0.00% 131 Missing :warning:
src/components/hooks/useSelect.tsx 10.34% 52 Missing :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #645 +/- ## ========================================== - Coverage 23.51% 22.13% -1.39% ========================================== Files 221 221 Lines 12607 13181 +574 Branches 235 236 +1 ========================================== - Hits 2965 2917 -48 - Misses 9553 10174 +621 - Partials 89 90 +1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

stropitek commented 7 months ago

It fixes the original problem but it introduces other ones. Keyboard navigation is broken

https://github.com/zakodium-oss/react-science/assets/4118690/a7af4251-e739-43ae-ade3-cdbc61a6e86e

wadjih-bencheikh18 commented 7 months ago

@stropitek Fixed, Could you please recheck categories-nested story. I found that it wasn't practical so I tried to improve it.

stropitek commented 7 months ago

Ref: https://github.com/zakodium-oss/react-science/issues/622

Did you base your resolution on https://github.com/palantir/blueprint/pull/4244#issuecomment-850554358?

wadjih-bencheikh18 commented 7 months ago

Did you base your resolution on palantir/blueprint#4244 (comment)?

This solution also have the same problem "Keyboard navigation is broken".

wadjih-bencheikh18 commented 7 months ago

Ref: #622

Fixed, Could you check please?

stropitek commented 7 months ago

Mouse interaction is broken

https://github.com/zakodium-oss/react-science/assets/4118690/19dc747a-b690-4ed9-b7ca-8a3d2228c89c

I think this component library is a great source of inspiration for how accessible components should work: https://react-spectrum.adobe.com/react-spectrum/Menu.html#dynamic

If possible we should try to mimic their behavior

stropitek commented 7 months ago

You can check what radix-ui has in terms of nested menus.

If blueprintjs is too broken maybe it's possible to mix the accessibility of radix-ui with the presentation of blueprintjs

wadjih-bencheikh18 commented 7 months ago

I fixed what you mentioned. Otherwise if you prefer other scenario please describe it.

wadjih-bencheikh18 commented 7 months ago

If blueprintjs is too broken

I think blueprintjs components are good but the examples in the documentation are not enough

stropitek commented 7 months ago

I fixed what you mentioned. Otherwise if you prefer other scenario please describe it.

It is good enough for now. Anyway I think nested menu aren't really a thing we should use in a select component. It makes more sense in a dropdown menu.