feat(menus): update Menu/Select Item indentation and focus based on item hover

[austingreendev]

• BREAKING CHANGE - HeaderItem styling changes and new focus/hover logic

Should also force a breaking change onto all dependent packages; rather than the default lerna behavior of marking it as a patch.

Description

• Removes an invalid active prop from a Menu example
• Adds containsIcon property for HeaderItem components (icon is a native attribute 😢 )
• Adds onMouseMove event to allow "focus on hover" mechanism
  • @allisonacs lets meet-up and go over this interaction pattern to double-check a few things

Checklist

• 👌 design updates are Garden Designer approved (add the
  designer as a reviewer)
• 💅 view component styling is based on a Garden CSS
  component
• 🌐 Styleguidist demo is up-to-date (yarn start)
• ⬅️ renders as expected with reversed (RTL) direction
• 💂‍♂️ includes new unit and snapshot tests
• 📒 any new files are included in the packages src/index.js export
• 📝 tested in Chrome, Firefox, Safari, Edge, and IE11

[coveralls]

Coverage decreased (-0.02%) to 95.811% when pulling 981bd74 on agreen/menu-indentation-styling into 7564c17 on master.

[ryanseddon]

How come onMouseMove over onMouseEnter the latter would only fire once on entering the menu item.

[austingreendev]

I originally had onMouseMove then, checked downshift for inspiration...

// onMouseMove is used over onMouseEnter here. onMouseMove
// is only triggered on actual mouse movement while onMouseEnter
// can fire on DOM changes, interrupting keyboard navigation

[allisonacs]

Before approving this, I wanted to flag something: when @jzempel and I met last week on this, I thought we decided to follow native implementation pretty much exactly. In this branch, you can still get into a situation where you have two items hovered/focused (mouse around, then use keyboard—you get the hover/focus both on the moused item, as well as on the item you keyed to. However, the native implementation of menus does not allow two hover/focus states: if you mouse, the hover follows your mouse, and if you then begin keyboarding, the keyboard steals the hover/focus.

I personally find it weird to have the two hover/focus states possible (I thought this was part of the decision to take away the visually distinct hover state).

[austingreendev]

I agree with @allisonacs, the dual styling is a little jarring.

This is a side-effect of the :hover styling being provided by the css-components. @jzempel is this possible to remove?

[jzempel]

Ah yeah, the CSS styling still needs some work 🙁 Actually, if possible, the styling should be updated locally to override &:hover with background-color: inherit; – applying only .is-hovered as appropriate.

[austingreendev]

I've updated the Item component with some custom styling and pre-published to https://garden.zendesk.com/react-components/menus/

There is some specificity weirdness, but it seems to be working as expected now. Let me know what you think.

[allisonacs]

Is it intentional that the old focus state is being used now?

[jzempel]

gnarly, but effective!

[jzempel]

@Austin94 on the demo page if you expand a menu, scroll it off the page, and click a button to expand a different menu ... the page jumps back to the original menu, collapses it, and the 2nd menu does not expand.

[austingreendev]

on the demo page if you expand a menu, scroll it off the page, and click a button to expand a different menu

This jumping is due to us returning focus to the element which triggered the menu. It aligns with most of menu implementations, including Material.

We could tweak this some though. A few options could be:

• Once a menu has been scrolled out of view entirely, close it without returning focus to it's trigger
  • Some complications with this is it depends on how popper.js is configured with it's bounding viewport

[austingreendev]

Is it intentional that the old focus state is being used now?

No, since I was doing a manual publish a fresh yarn install didn't happen. This is now updated at https://garden.zendesk.com/react-components/menus/

[jzempel]

@Austin94 consider a UI like dropbox (web). Expand one of the file menus in the list and then scroll away and expand a different file menu. You definitely wouldn't want the page to scroll back to the originally expanded menu – that would be a bad user experience. Similarly, we wouldn't want to see this kind of experience in a Zendesk table.

Furthermore, your Material example isn't ideal (I'd consider it to be an anti-pattern). Move from the "Week" menu over to either the product tray or notifications. The action is immediate. So the behavior isn't consistent in that UI.

Resolved offline