[RFC] MenuItem Children API

[miroslavstastny]

When implementing menu accessibility (#61), MenuItem (which by default renders as li > a) was modified and onClick handler as well as several styling were moved from li to a.

That works correctly for ShorthandApi, but unfortunately it breaks ChildrenApi. It was agreed previously that <Menu.Item>children</MenuItem> renders as <li>children</li>.
That worked previously as all the styles and onClick were on the li, but it is broken now.

I was thinking about splitting ListItem to ListItem (which renders as li) and ListItemLink (which renders as a). I played with the code to test this approach and it seems to work. The only problem I see is that a developer must pass the same props to several components:

<Menu>
  <Menu.Item>
    <Menu.Item.Link>
    </Menu.Item.Link>
  </Menu.Item>
</Menu>

Now if you want to make the menu to be vertical, you must add vertical to all Menu, Menu.Item and Menu.Item.Link. For an active item, you must add active to both Menu.Item and Menu.Item.Link.

What is our plan for supporting Children API?

[levithomason]

Shorthand only?

I am leaning toward not supporting children. It is at least worth considering for a first iteration.

In SUIR, we used to support both children and shorthand in the Dropdown. However, that component became so complex to use children that it didn't make sense for users. They needed to wire together so many keyboard handlers, accessibility checks, and stateful logic that it was simply not realistic.

We may find ourselves in a similar situation with some Stardust components, especially those with lists or lots of children. Sometimes the wiring for those children is too complex to expect users to do it correctly.

The strongest argument for this is that by using shorthand data driven components, users get a lot more for free:

• Always correct markup (regardless of variation defined)
• Always correct full accessibility
• Always correct full keyboard support
• Fully tested and documented feature support

Is shorthand enough?

I think if we do our shorthand props right, users won't need children. When rendering a component, there are a limited number of things to control. The component, props, the rendered tag name, and the tree structure:

1. Component: icon={{ as: MyIcon, name: 'user' }}
2. Props: icon={{ color: 'blue', name: 'user' }}
3. Tag: icon={{ as: 'i', name: 'user' }}
4. Tree: icon={(Icon, props) => <p><Icon {...props} /></p> }

These shorthand factory input values prevent the user from becoming blocked by not being able to use children.

Aside from what they need to control, we make it easy to do the most common thing. Such as define the most common prop via a primitive value or pass your own element:

1. Most common usage: icon='user'
2. Your own element: icon={<MyIcon name='user' />}

Many children, one renderer

The one thing we are missing right now is a way for components that have many children (List > Items, Menu > Items, Breadcrumb > Sections, ...) to have a single function for rendering those items. The typical pattern is that you have an array of items that you pass, however, it would be handy to have a single function that handled rendering each item:

const items = [{ key: 'a', content: 'Home', icon: 'home' }, ...{}]

<Menu
  items={items}
  renderItem={(Item, props) => <Item {...props} />}
/>

This would be an improvement that would make it even more justifiable, in my mind, to not support children initially.

---

Feedback?

[levithomason]

@brianespinosa / @layershifter / @fracmak - If possible, could you review the above convo regarding children vs shorthand and offer any input? I respect all of your engineering and experience much on this.

[brianespinosa]

I'll take a dig through my current application and see if I can find the places where we have opted for using the Child API versus Shorthand API. I think those Child API cases were because we needed to do some special/tricky work extending the internal markup of a component to meet some of our UI needs. In one case I believe it ended up being more work trying to do things this way and override some existing styles than to make a new custom component instead.

My initial inclination is that not supporting Child API at first is probably fine. And from a user experience perspective, I think a LOT of the questions we would get daily were from people not getting the out of the box functionality where it Just Works™.

I think if we DO end up supporting Child API we maybe explicitly cover that support in the docs with a warning that says using the Child API is for advanced use and some functionality will not magically work. Basically the Child API is you are mostly on your own and you have to know where prop drilling is happening, etc.

[fracmak]

I'll give it some thought, but I think we need to make certain we're consistent across all components. It's tough when you have to keep a mental list of what components are given shorthand and which use children. I personally really like the children api over the shorthand, but I do understand why it doesn't work for certain interactions (Dropdown is a good example). At which point it might make sense to do away with all children apis unless it's something like a popup or a modal where you are filling the component with your own content.

[miroslavstastny]

Menu currently supports shorthand API only