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

Remove old MenuButton, replaced with a new set of Menu components #278

Merged
merged 5 commits into from
Jul 30, 2020

Conversation

gnapse
Copy link
Contributor

@gnapse gnapse commented Jul 28, 2020

Short description

This introduces a new richer and accessibility-compliant dropdown menu.

It has support for

  • sub-menus
  • disabled menu items
  • optionally preventing closing the menu on certain item activation
  • adding titled groups of menu items
  • menu separators (simple <hr /> elements)
  • full accessibility support thanks to reakit (arrow keys nav, semantic elements recognized as menu and menu-items by screen readers, type-ahead some characters of an item's label to quickly move the focus to it).
  • individual elements are fully customizable. They can have applied their own styles, class names, or any supported DOM attribute. They can even be changed into being other kind of element (e.g. allowing the MenuButton be a reactist Button and receive its props, or a menu item can be a link, thus having a href and triggering navigation).

The menu items have a stricter format where you provide a label, icon (optional) and keyboard shortcut (optional) instead of free-form children. Though this is something I may change because after the fact I realize it may be too restrictive. I'll probably revert the menu items to allow more freedom in their content. The main Twist app menu is an example of the need for this. Update: menu items are now un-opinionated about their content. Much better that way.

I originally planned to not remove the existing MenuButton, merely deprecate it, and make this a minor release. But the name clash was what tipped it towards making this a replacement (can't export two different things named MenuButton), which of course means this is a breaking change.

There's a PR in the works that makes Twist adopt this new menu, replacing all the existing ones.

PR Checklist

  • Added tests for bugs / new features
  • Updated docs (storybooks, readme)
  • Executed npm run validate and made sure no errors / warnings were shown
  • Described changes in CHANGELOG.md
  • Bumped version in package.json
  • Updated all static build artifacts (npm run build-all)

Versioning

Should become part of a new major release (I'm exploring the possibility to make a single new major release for both this and #276 depending on the ability to synchronize having both merged around the same time).

Demo

CleanShot 2020-07-28 at 19 44 26

For the future

I have validated the possibility to implement some features in this menu that are needed in Todoist but not in Twist (e.g. using it as a context menu; allowing to control the open/closed state from the outside). I had some difficulties that should be solvable but I was already devoting too much time and the primary use case here is to be able to use this in Twist, where those features are not needed.

It would also be a nice to have to be able to have more semantic menu items such as checkbox menu item and a group of radio-button menu items. See reakit which has support for all this. We could adopt this for menu items such as Twist's "toggle dark/light theme" option in the main app menu.

@gnapse gnapse added dependencies Pull requests that update a dependency file DO-NOT-MERGE labels Jul 28, 2020
@gnapse gnapse self-assigned this Jul 28, 2020
@gnapse gnapse force-pushed the feature/menu branch 2 times, most recently from c9cdac4 to 38c1ddc Compare July 29, 2020 04:12
src/components/Menu.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@henningmu henningmu left a comment

Choose a reason for hiding this comment

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

Awesome, I played with in Storybook and it's on another level compared to what we have now 🚀

white-space: nowrap;
background: hsla(0, 100%, 100%, 0.99);
outline: none;
font-size: 85%;
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we rely on our font size constants (e.g. @smaller_font_size) here and let users override if necessary?

@gnapse gnapse marked this pull request as ready for review July 30, 2020 17:17
@gnapse gnapse merged commit 808516f into dev Jul 30, 2020
@gnapse gnapse deleted the feature/menu branch July 30, 2020 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants