feat(menu): Replace shape prop with pills, pointing and underlined props

[miroslavstastny]

Menu

Breaking change: this replaces shape='pills'|'pointing'|'underlined' with three separate props: pills, pointing and underlined.
The reason for that is that for vertical pointing menu, it must be possible to define pointer's direction (start vs end) and the single shape enum is not enough.

TODO

• Conformance test
• Minimal doc site example
• Stardust base theme
• Teams Light theme
• Teams Dark theme
• Teams Contrast theme
• Confirm RTL usage
• W3 accessibility check
• Stardust accessibility check
• Update glossary props table
• Update the CHANGELOG.md

API Proposal

<Menu pills ... />

<Menu pointing ... />

<Menu underlined ... />

[codecov]

Codecov Report

Merging #114 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master     #114   +/-   ##
=======================================
  Coverage   88.35%   88.35%           
=======================================
  Files          47       47           
  Lines         773      773           
  Branches      109      109           
=======================================
  Hits          683      683           
  Misses         87       87           
  Partials        3        3

Impacted Files	Coverage Δ
src/components/Menu/MenuItem.tsx	92.3% <ø> (ø)	⬆️
src/components/Menu/Menu.tsx	100% <100%> (ø)	⬆️

---

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 0dff885...02b0cb0. Read the comment docs.

[kuzhelov]

have discussed with Miro. So, essentially, we have the following conditions that define the task:

• shape is prop that accepts following values: pills | pointing | underlined
• vertical could be safely combined with any value of the shape prop
• 'pills', 'pointing' and 'underlined' shouldn't be applied at the same time (ideally we should have an API that would enforce it)

Problem

• for pointing we have additional set of values that could be applied - nothing (default one), 'start' or 'end'

Currently proposed solution

• split values of shape prop on severals props: pills, pointing, underlined
• as value of pointing prop the following could be provided: boolean, start, end

This leads us to the following things being possible

<Menu pointing ... />
<Menu pointing='start' ... />

As a drawback, we have no restrictions over client to do the following things (with, generally speaking, undefined result):

<Menu pointing underlined />  // mutually exclusive variants are mixed

Alternative solution

• consider to leave shape prop as it is
• introduce pointingPosition prop in addition

With that the following way of declaring pointing start menu should be used:

<Menu shape='pointing' pointingPosition='start' ... />

The main benefit of this approach is explicitness and consistency - there is still no way to mix exclusive variants of components, while complementary pointingPosition prop clearly suggests which shape it is designed for.

The drawback is mainly in amount of typing required - but change doesn't seem to be crucial.

[miroslavstastny]

I tend to agree with Roman. Although it seems to go against the concept of mixed properties, I see the benefits of enum.
@levithomason can you please share your opinion on this?

[levithomason]

A few thoughts on the Menu API:

shape and enum props

We had these same discussions at SUIR for our APIs. We also tried to implement shape in some components only to later revert them.

We found that props may move around a bit and they may later receive values that they didn't have before. This doesn't fit the enum approach.

There is also something to be said for concise APIs. Even though it may be more correct from a strict engineering perspective, we found some of the strictly correct APIs to be unfavorable to users. One example I recall was regarding discussion around <Message warning /> vs <Message type='warning' /> or <Message status='warning' />. The boolean prop here was preferred over the enum.

There is also HTML parity to consider. The evolution of React was based on the precedence of HTML. I think it is a good idea for frameworks to base their evolution on the same ideas as it makes adoption, understanding, and interoperability easier. In HTML, and therefore React, we have boolean props like disabled. These are not nested under a more correct attribute name such as state="disabled".

I would support boolean props for pills, pointing, etc. It leaves Stardust room to add values to them, it leaves theme producers room to allow users to combine them, it makes for a more readable and concise API, and I believe it is more approachable to a wider audience.

underlined

This prop is too coupled to the style implementation. We need to keep the API interface void of implementation details as much as possible. Otherwise, when implemented differently, the concept does not apply.

Case in point, what happens when you make an underlined menu also vertical? I would think the underline would move to the side, like this:

This is no longer an underlined menu. Instead, this could be a variant of the pointint type. This way, it can point in any direction. What the "pointing part" looks like is up to the theme, in this case, a line.

[miroslavstastny]

We had a discussion about that today, with questions like what defines the border between component API (props) and a theme (variables), why underlined is a prop not a variable...

When thinking about component API we must think out of box and consider not only different web themes but also completely different surfaces like native, even if we might never implement it, but just to find the correct abstraction.

I completely agree on the underlined being too coupled to the style - it's not a variant of a menu, it's a theme-specific way how to represent some menu variant. And in one theme this can be a representation of a pointing menu while in another theme it can be a representation of a secondary menu.

And, personally, I have the same opinion on pills.

[kuzhelov]

agreed to leave things as those are introduced (different props for each shape) due to reasons expressed above - if necessary, this point will be rethought in future.