Implement `<Menu>` component about ember-headlessui

[achambers]

Hey @GavinJoyce I'm trying to understand the context behind the menu/item-element component and how it relates to the menu/item component. What's the rationale behind it and the need for the two distinct components?

[GavinJoyce]

I don't remember the details. I seem to recall it was needed to keep parity with the Vue version, something to do with needing to have the flexibility of using a yielded item property on the element itself, eg:

<items.Item as |item|>
  <item.Element data-test-is-selected={{item.isActive}}>
    Item A
  </item.Element>
</items.Item>

[achambers]

I don't remember the details. I seem to recall it was needed to keep parity with the Vue version, something to do with needing to have the flexibility of using a yielded item property on the element itself, eg:

<items.Item as |item|>
  <item.Element data-test-is-selected={{item.isActive}}>
    Item A
  </item.Element>
</items.Item>

Hmm. Ok. I’ll take a look at the vue version to try and understand the rationale. On the face of it it looked like an unnecessary extra layer but I’m sure there was a reason behind it. I’ll try and uncover it, for my own knowledge if nothing else.

Thanks for the heads up man.

[GavinJoyce]

👍 I'd be delighted to remove the menu/item-element if we can, it's a pretty ugly API.

[achambers]

👍 I'd be delighted to remove the menu/item-element if we can, it's a pretty ugly API.

Yeh, I think we can. I can't see anything in the Vue component (https://headlessui.dev/vue/menu) that references the concept and I can't make out in my mind why we'd need it (could totally be missing something).

The only thing that seems close to this is this section which talks about rendering as template but I don't think it's really what you were going for. Is something we might want to look at implementing though.

We're going to be needing to use the Menu shortly and the Item.Element API is a bit annoying so I'll look at seeing what modifications I can make at that point, unless you get to it first. 👍 🙌

[GavinJoyce]

Nice. It's possible that the Vue API changed since I last looked at it late last year. It's also possible that I made it more complex that it needed to be. Either way, 🍺 if you manage to remove the need for it

[achambers]

Nice. It's possible that the Vue API changed since I last looked at it late last year.

Absolutely. I get the feeling you started this repo when the headlessui work was pretty much in its infancy.

All good my man, I'll see what I can do when I shift my focus to that component 👍

[alexlafroscia]

I spent a while yesterday thinking about the "menu item" vs. "menu element" problem and whether there's a way to remove the need for the element component. Here's how I'm wrapping my head around it right now:

• The "menu element" is actually the important part of the API. We need a DOM node that we can count on for a few things
  • Establishing where to place role="menuitem"
  • Placing the id for the item on an element that the menu can set as the "active descendant"
  • Read the text content for the purposes of activating a menu item when typing the first character(s) within it
• The "menu item" is useful as a way to yield the information about which "menu element" is active for styling purposes. Without it, the "menu element" doesn't really have a good way of being styled based on the active/inactive state

From looking at the implementation of the React components (and I'm assuming the Vue components are similar), they are able to look up the first child element of the "menu item" and automatically configure it with the attributes it needs, without needing a specific wrapper element. Ember doesn't give us any kind of API for a component to dynamically look at/configure it's children before they are rendered.

What could be done is changing the "menu element" component into a "menu element" modifier, that the user of Menu can attach to whatever existing DOM node they want to consider to be the menu element. This might be cleaner from an API perspective, but we lose the benefits of the declarative API we have for configuring the "menu element" right now.

So, if an API like this might be better:

<Menu as |menu|>
  <menu.Items as |items|>
    <items.Item as |item|>
      <span {{item.setElement}} class={{if menu.isActive "bg-blue"}}>
        I am the menu item!
      </span>
    </items.Item>
  </menu.Items>
</Menu>

and we're OK with imperatively setting attributes and event listeners, we could make that API change!

Unfortunately I don't think there's any sort of compositional API for combining modifiers, so we'd need to essentially re-implement the {{on}} modifier (which might not be that bad, and maybe there's some way we could import the implementation to consume it from JS? That seems like a hack, though)

[alexlafroscia]

BTW

Type to select

This is done as of #57

[GavinJoyce]

Thanks for digging into this @alexlafroscia and for refreshing my memory on why I initially added two components for each item.

If we can't find a way to collapse the definition of an item into a single component, I personally don't see much of a difference between these two APIs:

two components:

<Menu as |menu|>
  <menu.Items as |items|>
    <items.Item as |item|>
      <item.Element @tagName="span" class={{if menu.isActive "bg-blue"}}>
        I am the menu item!
      </item.Element>
    </items.Item>
  </menu.Items>
</Menu>

one component and one modifier:

<Menu as |menu|>
  <menu.Items as |items|>
    <items.Item as |item|>
      <span {{item.setElement}} class={{if menu.isActive "bg-blue"}}>
        I am the menu item!
      </span>
    </items.Item>
  </menu.Items>
</Menu>

but we lose the benefits of the declarative API we have for configuring the "menu element" right now.

If there's a downside to using a modifier, perhaps the existing API is preferable?

[alexlafroscia]

If there's a downside to using a modifier, perhaps the existing API is preferable?

I think you're right!

One API clean-up thing that we could look into is doing something more like this:

<Menu as |menu|>
  <menu.Items>
    <menu.Item as |item|>
      <menu.ItemElement @tagName="span" class={{if menu.isActive "bg-blue"}}>
        I am the menu item!
      </menu.ItemElement>
    </menu.Item>
  </menu.Items>
</Menu>

(Specific names certainly can be bike-shedded)

There's a few nice things here:

• All components are rendered off of menu, so there's less need to yield in the templates. It also matches the React/Vue naming a little more closely, where all the components are Menu or Menu.Something
• If the user doesn't need to know which element is active in the template -- maybe because they are going to use the ARIA attribute to style the active one differently or something -- then they could presumable leave the menu.Item component out altogether, since all it really does is yield data right now