feat(Menu): Add wrapper prop to MenuItem by miroslavstastny · Pull Request #323 · microsoft/fluent-ui-react

miroslavstastny · 2018-10-05T13:21:13Z

Previous approach (MenuItemLink)

This is an alternative proposal to #283.
The original proposal splits MenuItem (ElementType > a = li > a) to MenuItem (li) and MenuItemLink (a).
That gives us a lot of flexibility, but makes Menu API more difficult:

user needs to use nested objects: { content: {content: 'github.com', href:'https://github.com'}}
it is not clear which properties should go to which object: { content: {icon: 'call'}, icon: 'home' }
user needs to know where to put accessibility attributes or accessibility behavior needs to 'move' the attributes to the inner a (but then it is not possible to specify the attribute for li)

That approach also adds additional React component to the tree.

`MenuItem` `wrapper` slot

This PR keeps MenuItem as a single element but renders it as wrapper > ElementType
That means that the a is now the 'main element'

Wrapper benefits

Considering ul > li > a menu structure, users would need to customise the a more often than li. This proposal makes customisations to a straightforward while keeping customisations to the li possible.

MenuItemLink vs Wrapper comparison

	MenuItemLink	wrapper
content	`{content: 'Home' }`	`{content: 'Home' }`
icon	`{content: {content: 'Home'}, icon: 'home' }` `{content: {content: 'Home', icon: 'home'}}`	`{ content: 'Home', icon: 'home'}`
href	`{ content: {content: 'github.com', href:'https://github.com'}}`	`{ content: 'github.com', href:'https://github.com'}`
`as` for inner	`{ content: {as: Button, content:'Home'}}`	`{as: Button, content:'Home'}`
`as` for outer	`{as: 'div', content:'Home'}`	`{content: 'Home', wrapper: <div/>}`
custom outer		`{content: 'Home', renderWrapper: (Component, props) => (<Component {...props} style={{border: '1px solid red'}} />)}`
remove outer		`{content: 'Home', wrapper: <React.Fragment/>}`
custom `role` for outer		`{ content: 'Home', wrapper: {role: 'custom-role'} }`

Tests

Both isConformant and handlesAccessibility now support components with wrapper slot using usesWrapperSlot config.

codecov · 2018-10-05T13:21:17Z

Codecov Report

Merging #323 into master will decrease coverage by 0.02%.
The diff coverage is 72.72%.

@@            Coverage Diff             @@
##           master     #323      +/-   ##
==========================================
- Coverage    88.4%   88.37%   -0.03%     
==========================================
  Files          41       41              
  Lines        1423     1428       +5     
  Branches      181      207      +26     
==========================================
+ Hits         1258     1262       +4     
- Misses        161      162       +1     
  Partials        4        4

Impacted Files	Coverage Δ
src/components/Menu/MenuItem.tsx	`80.48% <72.72%> (-0.07%)`	⬇️

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 4933d5e...c6ae0cc. Read the comment docs.

kuzhelov · 2018-10-05T13:43:25Z

share the same sentiments around benefits of the proposal (as well as about the problems it addresses), need to make couple of tests to verify it further. Speaking of the tests - this is the thing that is easily adjustable, as long as we will agree to update already taken principles :) But, once again, as it is relatively drastic change, it should be carefully tested before.

Just for the sake of having a fair overview: in regards to impossibility of addressing customOuter case with the shorthand - is it really the case? Won't it be possible to achieve this functionality by using function shorthand, like

...={() => (Component, props) => (<li {...props} style={{border: '1px solid red'}}>{props.children}</li>)}}

The same would apply to removeOuter's case.

…nd handlesAccessibility, re-enabled MenuItem tests

bmdalex

Great work @miroslavstastny

test/specs/commonTests/isConformant.tsx

jurokapsiar · 2018-10-10T13:32:50Z

src/lib/factories.tsx

 export const createHTMLInput = createShorthandFactory('input', val => ({ type: val }))
 export const createHTMLLabel = createShorthandFactory('label', val => ({ children: val }))
 export const createHTMLParagraph = createShorthandFactory('p', val => ({ children: val }))
+export const createHTMLLi = createShorthandFactory('li', val => ({ children: val }))


createHTMLListItem would be more consistent with other factory names

miroslavstastny · 2018-10-10T16:12:16Z

One additional note - As discussed with @jurokapsiar offline, if this approach gets merged, we will review MenuItem behaviors and simplify them as a lot of their logic is no longer necessary after this change.
We will be probably able to completely remove handledProps concept from accessibility.

This PR fixes #191.
This PR obsoletes #283.

bmdalex · 2018-10-10T17:01:08Z

src/components/Menu/MenuItem.tsx

    )
+
+    if (wrapper) {
+      return createHTMLListItem(wrapper, {


don't we want this to be an actual Slot shorthand so it's being passed through the Fella renderer? look for Slot.create references; we can discuss tomorrow

To expand here, we need to pass the content of wrapper prop through the Fella renderer in order to leverage Fella's functionality (rtl, styles applied as classnames, etc)

The way to go now is by using Slot; I'm doing the same for Input in #326

Right now we need to use wrapper.style instead of wrapper.styles to style the wrapper and these are applied directly to element.style and not through classnames:

wrapper.styles won't work (it will be added directly to the DOM as an attribute), need to use Slot for that:

bmdalex · 2018-10-10T20:40:29Z

src/components/Menu/MenuItem.tsx

+    if (wrapper) {
+      return createHTMLListItem(wrapper, {
+        defaultProps: {
+          className: classes.root,


there are a few problems caused by this; when we pass custom styles prop at the MenuItem level, they will be applied to the li wrapper instead of the a link

It can be fixed by renaming the root slot to something like wrapper here and add classes.root to the a link; here are the changes:

and here is the expected result (contains the changes):

problem is the tests will fail now..

Good catch, thanks!
The main problem is that ui-menu__item class now goes to the a and nothing to the li.
Not sure if it would be ok to have ui-menu__item on the inner a and ui-menu-wrapper on the outer li.
Plus similar change should be done in behaviors.
I will look into that.

well we can definitely rename ui-menu__item to ui-menu__item__link or something similar and use item instead of wrapper an name that ui-menu__item instead of ui-menu-wrapper

here is another example that shows that styles prop and wrapper.style are styling the same element, the wrapper and wrapper.style with the latter having higher priority since it's applied to the element style directly; currently we cannot target the a link to style it:

… to a

miroslavstastny · 2018-11-01T14:46:27Z

I've updated the PR as follows:

Slot is used for wrapper
in styles, inner anchor is considered 'root', wrapper is 'wrapper'
inner anchor class is ui-menu__item, wrapper class is ui-menu__item__wrapper
every prop applied on MenuItem component goes to anchor (including style and styles), use wrapper prop to alter the li:

Example

const items = [
...
  {
    key: 'review',
    content: 'Reviews',
    styles: { borderLeft: '5px solid red' },
    style: { background: 'pink' },
    className: 'CLASS-ANCHOR',
    'data-custom': 'DATA-ANCHOR',
    wrapper: {
      styles: { paddingLeft: '50px' },
      style: { background: 'yellow' },
      className: 'CLASS-WRAPPER',
      'data-custom': 'DATA-WRAPPER'
    },
  },
...
]

Renders as:

<li role="presentation"
    style="background:yellow"
    data-custom="DATA-WRAPPER"
    class="ui-slot dy ui-menu__item__wrapper CLASS-WRAPPER">

     <a class="ui-menu__item ea CLASS-ANCHOR"
        role="menuitem"
        tabindex="0"
        data-is-focusable="true"
        style="background:pink"
        data-custom="DATA-ANCHOR">
          Reviews
     </a>
</li>

@Bugaa92 would you mind reviewing again? Thanks!

bmdalex

pls take a look at comments, thanks

bmdalex · 2018-11-01T15:36:00Z

src/components/Menu/MenuItem.tsx


  static defaultProps = {
-    as: 'li',
+    as: 'a',


when we discussed changes for Input component we had a consensus that as and styles props always go to the root element; we might want the same for MenuItem; let's see what others say @levithomason @kuzhelov

IIRC there is an accessibility requirement that the click listener needs to be on the anchor tag. The primary use case for as on the menu item is to allow adding router functionality. This means the router's navigation handler needs to be applied to the anchor:

import { NavLink } from 'react-router' <Menu items={[{ as: NavLink, to: '/', content: 'Home' }]} />

If the as prop goes to the root, it will still work as the event will bubble from the anchor up to the li, however, it would not be the correct markup and behavior for the accessibility requirement.

bmdalex · 2018-11-01T15:39:49Z

src/components/Menu/MenuItem.tsx

+      children
+    ) : (
      <ElementType
        className={classes.root}


when we discussed changes for Input component we had a consensus that styles prop always goes to the root element; we might want the same for MenuItem; this will break that convention when wrapper prop is defined; let's see what others say @levithomason @kuzhelov

bmdalex · 2018-11-01T15:44:28Z

src/lib/factories.tsx

 export const createHTMLInput = createShorthandFactory('input', val => ({ type: val }))
 export const createHTMLLabel = createShorthandFactory('label', val => ({ children: val }))
 export const createHTMLParagraph = createShorthandFactory('p', val => ({ children: val }))
+export const createHTMLListItem = createShorthandFactory('li', val => ({ children: val }))


no need to add this as we're removing HTML factories in #376

bmdalex · 2018-11-01T15:45:27Z

src/components/Slot/Slot.tsx


  static create = createSlotFactory(Slot.defaultProps.as, content => ({ content }))
  static createHTMLInput = createSlotFactory('input', type => ({ type }))
+  static createHTMLListItem = createSlotFactory('li', children => ({ children }))


no need to add this as we're removing HTML factories in #376
use createSlotFactory('li', children => ({ children })) directly where you need it

bmdalex · 2018-11-01T15:46:37Z

src/components/Menu/MenuItem.tsx

    )
+
+    if (wrapper) {
+      return Slot.createHTMLListItem(wrapper, {


this was discussed in previous sync meetings; pls use createSlotFactory('li', children => ({ children })) directly; see comments in Slot.tsx

bmdalex · 2018-11-01T15:56:22Z

test/specs/commonTests/isConformant.tsx

      }
    }
+
+    if (usesWrapperSlot) {


@kuzhelov @levithomason
I think we should avoid changing isConformant tests for the elements with wrapper slot; we had this discussion for Input component.

@miroslavstastny AFAIR, the tests that fail are related to as and className props not being set correctly; these can be solved by addressing the comments I had in MenuItem.tsx where as and styles should always be passed to the root element, being that the a or the li for the MenuItem case.

This probably requires a session where we can discuss options in more detail; @miroslavstastny feel free to schedule a meeting with me and @kuzhelov

Thank you very much for the review, @Bugaa92.

So the main question is whether we should split props between li and a as we do in Input or pass everything to the a.

I am not strictly against doing the split, but when I was considering this I thought it would make more sense to do 'everything to a' although it is different approach than in Input.

We should consider usability and API consistency first and test after that.

Agree to discuss this offline.

thanks for sharing your thoughts, @miroslavstastny, @Bugaa92

So the main question is whether we should split props between li and a as we do in Input or pass everything to the a.

If I would answer this question only for the MenuItem component I would rather choose to not split and apply all the props to wrapped <a> element. This would make the following usage case be quite more apparent to me as a user, given that I am aware of wrapper concept

<MenuItem styles={..} href={..} />

It would be really confusing and non-intuitive to me if href will be applied to <a> element, while, at the same time, styles would be applied to <li>. In this situation I would rather expect consistent behavior, i.e. both props being applied to <a>.

At the same time, agree with the @Bugaa92's reasons for the concerns he has expressed, although see Input's situation to be a bit different: while for the menu's case it is a web standard that declares li to be a wrapper, in contrast, for the Input component's case it is a Stardust's need to use wrapper div (what is more interesting, this one is necessary for styling aspects, a bit smelly fact to me).

Agree to strive for consistent and intuitive API, and for this sake, let's, please, schedule a meeting as you've proposed - I will be back staring from 7th of November, but let me know if it should be decided earlier, will do all my best to allocate time for that.

Thank you!

bmdalex · 2018-11-16T11:35:37Z

src/components/Menu/MenuItem.tsx

+        },
+        render: renderWrapper,
+        overrideProps: () => ({
+          children: menuItemInner,


very nice usage of overrideProps for children in the wrapper; basically we achieve not allowing the user to override children through wrapper prop 👍
I'll create a PR to do the same for Input component

In this case we can use overrideProps as object

src/components/Menu/MenuItem.tsx

feat(Menu): Add wrapper prop to MenuItem

81c9fe2

miroslavstastny added help wanted Extra attention is needed RFC labels Oct 5, 2018

miroslavstastny requested review from dvdzkwsk, kuzhelov, levithomason and mnajdova as code owners October 5, 2018 13:21

kuzhelov added 🚧 WIP and removed help wanted Extra attention is needed labels Oct 9, 2018

Merge remote-tracking branch 'origin/master' into feat/menu-item-wrapped

0334bb2

miroslavstastny removed the RFC label Oct 10, 2018

miroslavstastny added 2 commits October 10, 2018 12:03

- added renderWrapper, added render wrapper support in isConformant a…

faefd9f

…nd handlesAccessibility, re-enabled MenuItem tests

Merge remote-tracking branch 'origin/master' into feat/menu-item-wrapped

af5872f

miroslavstastny added 🚀 ready for review and removed 🚧 WIP labels Oct 10, 2018

bmdalex approved these changes Oct 10, 2018

View reviewed changes

bmdalex reviewed Oct 10, 2018

View reviewed changes

test/specs/commonTests/isConformant.tsx Show resolved Hide resolved

jurokapsiar reviewed Oct 10, 2018

View reviewed changes

jurokapsiar approved these changes Oct 10, 2018

View reviewed changes

- address PR comments

67d1d82

bmdalex reviewed Oct 10, 2018

View reviewed changes

kuzhelov added 🚧 WIP and removed 🚀 ready for review labels Oct 15, 2018

miroslavstastny added 3 commits October 31, 2018 20:50

Merge remote-tracking branch 'origin/master' into feat/menu-item-wrapped

7f2599c

fix tests after merge

045d4dd

MenuItem uses Slot for wrapper, MenuItem style and styles are applied…

e1600b2

… to a

miroslavstastny added 🚀 ready for review and removed 🚧 WIP labels Nov 1, 2018

Merge remote-tracking branch 'origin/master' into feat/menu-item-wrapped

521062e

bmdalex suggested changes Nov 1, 2018

View reviewed changes

miroslavstastny mentioned this pull request Nov 5, 2018

feat(Menu): MenuItemLink component #283

Closed

bmdalex approved these changes Nov 7, 2018

View reviewed changes

levithomason approved these changes Nov 14, 2018

View reviewed changes

miroslavstastny added 3 commits November 16, 2018 09:35

Merge remote-tracking branch 'origin/master' into feat/menu-item-wrapped

8980cd3

- fixed wrapper slot

17773e3

Merge remote-tracking branch 'origin/master' into feat/menu-item-wrapped

76a9d5d

bmdalex reviewed Nov 16, 2018

View reviewed changes

src/components/Menu/MenuItem.tsx Show resolved Hide resolved

bmdalex approved these changes Nov 16, 2018

View reviewed changes

bmdalex added ready for merge and removed 🚀 ready for review labels Nov 16, 2018

miroslavstastny added 2 commits November 16, 2018 12:51

- changelog

c3d4e16

Merge remote-tracking branch 'origin/master' into feat/menu-item-wrapped

c6ae0cc

miroslavstastny merged commit 25fa95d into master Nov 16, 2018

miroslavstastny mentioned this pull request Nov 16, 2018

Menu item href breaks focus (necessary to TAB twice) #191

Closed

layershifter deleted the feat/menu-item-wrapped branch January 10, 2019 11:26

Conversation

miroslavstastny commented Oct 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Previous approach (MenuItemLink)

MenuItem wrapper slot

Wrapper benefits

MenuItemLink vs Wrapper comparison

Tests

Uh oh!

codecov bot commented Oct 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kuzhelov commented Oct 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bmdalex left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miroslavstastny commented Oct 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miroslavstastny commented Nov 1, 2018

Uh oh!

bmdalex left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

miroslavstastny commented Oct 5, 2018 •

edited

Loading

`MenuItem` `wrapper` slot

codecov bot commented Oct 5, 2018 •

edited

Loading

kuzhelov commented Oct 5, 2018 •

edited

Loading