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

[WIP] Use Reach UI for dropdown menus #158

Open
wants to merge 1 commit into
base: sfh/update-deps
Choose a base branch
from

Conversation

solomonhawk
Copy link

@solomonhawk solomonhawk commented May 11, 2020

WORK IN PROGRESS

This PR pulls in @reach/menu-button and replaces usage of react-focus-trap for BlockMenu and BlockTypeGroup.

Addresses #155

Notes:

  • Adopting Reach UI Menu has a few implications tied to the philosophy they take on a11y:
    • Components are black-boxed: it's difficult to introspect the state of the menu, which is intentional, and there's no programmatic API for interacting with it
    • Positioning of the popover is only partially configurable/controllable since the library does a lot of work to place it correctly (including during/after scrolling and resizing events)
    • As a result, our dropdown menus no longer appear overlapping the buttons that trigger them. Instead the dropdown is positioned below (or above, in certain cases) the trigger
    • Of course it's worth mentioning that what we get in exchange is accessibility (including keyboard up/down behavior, which is a nice win)
  • I ran into some trickiness w/ feature parity initially - specifically around Escape key handling around the menu (Reach UI makes it hard to introspect the state of the dropdown which we need to decide whether to stop propagation of a key up event). I worked around this using some less-than-ideal patterns and made a note of that in the code.
  • Updating the tests was also tricky. I found it easiest to pull in some pieces from @testing-library (see note below, would be good to swap those in wholesale)
  • Upgrades sass-loader to 8.x
  • Upgrades style-loader to 1.2.x

TODO:

  • figure out whether I can get react-ink back into the menu buttons (I think it's doable)
  • replace react-dom/testing-utils in tests w/ @testing-library/react

@codecov-io
Copy link

Codecov Report

Merging #158 into sfh/update-deps will decrease coverage by 0.41%.
The diff coverage is 26.66%.

Impacted file tree graph

@@                 Coverage Diff                 @@
##           sfh/update-deps     #158      +/-   ##
===================================================
- Coverage            93.72%   93.31%   -0.42%     
===================================================
  Files                   34       34              
  Lines                  303      299       -4     
  Branches                43       42       -1     
===================================================
- Hits                   284      279       -5     
- Misses                  17       18       +1     
  Partials                 2        2              
Impacted Files Coverage Δ
src/components/BlockTypeGroup.js 13.33% <0.00%> (-0.96%) ⬇️
src/components/MenuItem.js 100.00% <ø> (ø)
src/components/Block.js 91.66% <100.00%> (-0.65%) ⬇️
src/components/BlockMenu.js 100.00% <100.00%> (ø)
src/components/MenuHandle.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a9177df...b5e2db5. Read the comment docs.

@solomonhawk solomonhawk changed the title [WIP] [deps][a11y] Use Reach UI for dropdown menus [WIP] Use Reach UI for dropdown menus May 11, 2020
@solomonhawk solomonhawk marked this pull request as draft May 11, 2020 15:28
@solomonhawk solomonhawk marked this pull request as ready for review May 11, 2020 15:28
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