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

Menu Component Improvements #58

Merged
merged 3 commits into from
Jul 5, 2021

Conversation

alexlafroscia
Copy link
Collaborator

@alexlafroscia alexlafroscia commented Jun 26, 2021

Menu Avoids Rendering a DOM Node

The Menu component in the React/Vue implementations does not render a DOM node by default. It can, because I believe they want to make sure that all components can use an as= property to configure the node that's rendered, but by default it just renders a Fragment (which results in nothing in the DOM).

Since Ember doesn't really have a concept of a Fragment, I think that not rendering a DOM node at all is closer to the React/Vue implementation than rendering a div.

Note: This is a breaking change, as any attributes or modifiers placed on this element should be moved to a new parent div element

Before

<Menu data-test-menu class="bg-gray" as |menu|>
  ...
</Menu>

After

<div data-test-menu class="bg-gray">
  <Menu as |menu|>
    ...
  </Menu>
</div>

Menu::Items Focusing on Render

From the Headless UI documentation on the Menu:

Clicking the MenuButton toggles the menu and focuses the MenuItems component.

Using the focus trap here is a little unintuitive, because the menu item elements themselves are not focusable. However, this is a useful approach because

  • Focus should be locked within the menu anyway, per the accessibility requirements of a menu
  • Focus needs to be returned to the previously-focused element when the menu closes, which the focus-trap modifier will do for us

The documentation for the focus-trap library describes how you should handle a case where the element that focus is trapped within has no focusable children, which is to focus the "wrapper" element itself using initialFocus, which is the approach taken here!

Using the focus-trap library to return focus to the trigger button should resolve the weirdness around focus returning to the wrong element sometimes. focus-trap will also handle the "press-escape-to-dismiss-the-menu" behavior for us without needing additional logic for that.

Closes #55
Closes #54

@alexlafroscia
Copy link
Collaborator Author

Something isn't quite right with the way the focus trap is being used -- I'll mark this is ready once I fix that up!

@alexlafroscia alexlafroscia force-pushed the menu-improvements branch 2 times, most recently from 0049925 to 1008117 Compare June 28, 2021 14:20
@alexlafroscia alexlafroscia changed the title fix(menu): focus menu on open Menu Component Improvemenets Jun 28, 2021
@alexlafroscia alexlafroscia changed the title Menu Component Improvemenets Menu Component Improvements Jun 28, 2021
This change brings the Ember implementation closer inline with the React/Vue implementations, which do not create a DOM node for this component automatically. This makes working with the component easier, as it no longer introduces un-necessary DOM nodes.

Note: the React and Vue implementations allow for optionally creating a DOM node for the `Menu` component. This is possible in Ember, technically, but probably more trouble than it's worth, since the user of `Menu` can just wrap the component in an additional DOM node themselves if that's what they want. I believe the React and Vue implementations include this functionality for consistency, so that _all_ Headless UI components can optionally render an alternate DOM node, but those frameworks have an easier ability to pull this off than Ember does.

A focus trap has been introduced to power the click-outside-to-dismiss functionality, which also resolves the issue of focus needing to be locked in the `Menu::Items` component when rendered, which was previously not the case.

BREAKING CHANGE: If you were previously depending on `Menu` rendering a DOM node, you'll want to wrap your usage of `Menu` with a `div` and move any attributes or modifiers to that element instead.
Copy link
Owner

@GavinJoyce GavinJoyce left a comment

Choose a reason for hiding this comment

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

I think the existing API is a little nicer to work with, but 👍 on the change if it brings us closer to the React/Vue APIs

@alexlafroscia
Copy link
Collaborator Author

alexlafroscia commented Jun 30, 2021

I think the existing API is a little nicer to work with

Just to clarify, you'd prefer that Menu does have a DOM node of it's own?

I'm thinking about a case where someone builds out a component that wraps Menu. If it were me, I would do something like this:

{{! app/components/my-menu.hbs }}
<Menu as |menu|>
  <menu.Trigger
    {{did-insert (set this 'menuTrigger')}}
    ...attributes
  />
  <menu.Items
    {{popper-tooltip this.menuTrigger}}
  />
</Menu>

If Menu creates a DOM node of it's own, I probably need some way to add classes to both Menu and the menu trigger since that might end up being necessary to get the layout where this component is used correct. Needing to think about both of these nodes is unnecessary overhead from the perspective of someone making use ofMenu. If I want that wrapper node, it's easy to wrap this whole thing in a div, but I bet in most cases it would be better to conceptually map my component to a single DOM node, the menu trigger.

@alexlafroscia
Copy link
Collaborator Author

alexlafroscia commented Jun 30, 2021

I'd love to get a review from @achambers as well before merging, just to make sure everyone's on the same page with the changes here since it's a breaking change!

Edit: Upon thinking about it more, I'd love verification that this indeed closes the two tickets that I linked to as well. I'm pretty sure it does but would love to make sure!

@GavinJoyce
Copy link
Owner

GavinJoyce commented Jul 1, 2021

I'm thinking about a case where someone builds out a component that wraps Menu. If it were me, I would do something like this

👍 I'm convinced by your example, thanks

Copy link
Collaborator

@achambers achambers left a comment

Choose a reason for hiding this comment

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

Love your work @alexlafroscia. I'm in support of this improvement. Thanks.

@alexlafroscia alexlafroscia merged commit 9b1c513 into GavinJoyce:master Jul 5, 2021
@alexlafroscia
Copy link
Collaborator Author

Thanks @achambers !

@achambers achambers added the breaking PR is considered a breaking change label Jul 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking PR is considered a breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Focus of menu button on close seems incorrect. <Menu> doesn't close items dropdown on Escape
3 participants