feat: Menu refactor to Fiori3 #702

kavya-b · 2020-02-25T07:08:39Z

Related Issue

Closes #571

Description

Update Menu styles, documentation and tests to align with Fiori3 specs

removed fd-menu__group classes, their playground tests and documentation examples related to group headers.
added new classes for compact and mobile modes: fd-menu--compact and fd-menu--mobile. Default will be cozy tablet mode.
moved fd-menu__item--separated from <ul> level to its own element fd-menu__separator in a  since separators are now used instead of group headers to show grouping.
using existing class fd-menu__title for the text that will be used in a 
added new class for secondary icon fd-menu__addon-after that will be used similar to fd-menu__addon-before
added a modifier class for the affordance arrow when used in a submenu fd-menu__addon-after--submenu to get the right(or left in RTL) navigation arrow in a smaller icon size.
added new class for submenu fd-menu__sublist at the <ul> level corresponding to fd-menu__list of the parent menu.
added new class fd-menu__link that wraps the item name and associated icons, if any.
added new class has-child that can be applied on the parent __link class to apply styles for the parent element that has a submenu.
moved fd-menu__item class to the parent <li> tag.

Breaking changes

Removed fd-menu__item--separated. Instead use fd-menu__separator in its own  after the <li> item where you want the separator.
Removed fd-menu__group. Separator itself acts a visual cue for grouping.
Removed fd-menu--addon-before(not fd-menu__addon-before) as it is unnecessary now and will be covered by existing classes.
Moved fd-menu__item class to the parent <li> tag. Use fd-menu__link class on the <a> tag instead.

For new classes added, please refer to the above section.

Screenshots

NOTE: If you've made any style changes, please provide appropriate screenshots (before and after) to help reviewers.

To see examples of which screenshots to include, go to Screenshot Examples.

Before:

After:

Updated screenshots:

netlify · 2020-02-25T07:25:03Z

Deploy preview for fundamental-styles ready!

Built with commit ee0c701

https://deploy-preview-702--fundamental-styles.netlify.com

JKMarkowski

Let's add cozy mode by default, because right now there is only compact mode.
There are also differencted between Compact/Cozy(tablet)/Cozy(Phone)
Background color of fd-menu is not set
Remove underline from <a> if used with __title

src/menu.scss

JKMarkowski

Let's remove some of unused/useless properties. They can cause some issues in future. Please also test the changes on IE11, which has to be supported as well.

src/menu.scss

rengare

The menu item has text decorator this doesn't match fiori 3

wrong width in IE 11

Could you add RTL example to documentation?

kavya-b · 2020-02-27T12:48:46Z

The menu item has text decorator this doesn't match fiori 3

This is coming from the styles <a> tag in documentation itself. It appears fine in playground:

Currently same thing is happening for side-navigation component as well.

wrong width in IE 11

I will work on this; however, in the meanwhile, do you think this is related to #691 ?

Could you add RTL example to documentation?

I will add this.

kavya-b · 2020-02-27T13:02:19Z

Let's add cozy mode by default, because right now there is only compact mode.

Do you mean, I should have the cozy mode applied by default? But for desktop, it is mentioned as Compact mode, right?

There are also differencted between Compact/Cozy(tablet)/Cozy(Phone)

Currently, the styles of cozy, compact are applied when the screen is resized. However, from your other comment, it seems that these styles are better provided as separate fd-menu-cozy fd-menu-compact classes. I will work on changing this.

Background color of fd-menu is not set

I assumed that since fd-menu will be part of a popover, the background color, box shadow and border attributes of the container should be part of the popover. I have verified that the appropriate values are applied to the popover class.

Remove underline from <a> if used with __title

This is coming from the styles <a> tag in documentation itself. It appears fine in playground:

Currently same thing is happening for side-navigation component as well.

rengare · 2020-02-28T12:43:46Z

I will work on this; however, in the meanwhile, do you think this is related to #691 ?
Frankly, I don't know, we know that it is due to the popover and input used together but maybe it is related. I would have to check that

CLAassistant · 2020-03-03T16:36:09Z

All committers have signed the CLA.

kavya-b · 2020-03-04T05:44:16Z

There are 2 known issues in IE as of now that I couldn’t find a fix for but I also see the same issue is present in components that use List component(Select, Multi Input etc)-

focus border outline-offset property is not applied properly in IE due to which it sometimes does not appear properly
clicking on the parent <li> area alone activates the states (active, selected etc). If you click on an area that is a child of <li> the states don’t show up.

stefanoScalzo · 2020-03-05T01:05:47Z

disabled should not change color on hover and on click

kavya-b · 2020-03-05T08:59:43Z

disabled should not change color on hover and on click

fixed

JKMarkowski

Hello @kavya-b, please take a look at this comment, Rest looks good for me.

JKMarkowski · 2020-03-09T15:35:45Z

src/menu.scss


-    display: block;
+    display: flex;
+    align-items: center;
    transition: all $fd-animation-speed ease-in;


There is no mention about transition in menu specs

Missed this. Will remove it, thanks for pointing it out!

droshev · 2020-03-18T18:34:07Z

@kavya-b thanks for the updates.
Could you try to review your component following the points from https://github.com/SAP/fundamental-styles/wiki/Self-Contained-Styles. There is something wrong with the separator:

kavya-b · 2020-03-19T06:24:13Z

https://github.com/SAP/fundamental-styles/wiki/Self-Contained-Styles

Hi @droshev, I tried to figure out if I am missing something that makes the unnormalized mode look like that, but cannot point to anything.
fd-menu__separator is in its own element in order to cover the edge case where we have a selected item and also need to show the separator, as pointed by @InnaAtanasova in this comment: #702 (review)

Update:
Although by specifying it as an element rather than a modifier(using __separator rather than --separator, I see that this unnormalized behavior is fixed only if I specify fd-menu fd-menu__separator in my .njk file:

I do not quite understand why this is happening, as by itself it works just fine in the documentation pages. However, for the tests I will be providing it like the aforementioned fix.

droshev · 2020-03-22T21:57:48Z

@kavya-b We need to check how the nested menu behaves and looks on mobile. I guess it may behave like the nested list in side navigation. Can you follow up on that?
Meanwhile i fixed the self-contained issue I had raised before.

kavya-b · 2020-03-23T06:57:06Z

@kavya-b We need to check how the nested menu behaves and looks on mobile. I guess it may behave like the nested list in side navigation. Can you follow up on that?

Hi @droshev , the spec says "On Phone the Submenu opens in a new page, leaving the Menu option behind. " The same visual design will apply for the submenu; on clicking of the parent item, the submenu will open in the same dialog, replacing the parent menu options, and the device's back button will bring it back to the parent menu.
Does this need to be added to the documentation? I am not sure how to show a full screen replacement of parent menu in the same example using only aria-controls. I have something like this now:

Please let me know if this suffices, I will push in the changes.

Meanwhile i fixed the self-contained issue I had raised before.

Thank you for this.

kavya-b · 2020-03-26T03:14:19Z

@droshev As discussed, the documentation for mobile mode has been modified to reflect full screen:

droshev

🚢 Looks good to me! good work

addressed all changes requested

…bile mode - keeping tablet cozy as the default - added rtl example - fixed IE submenu width issue - changed option-name to title - removed float usage - provided examples for cozy, compact and mobile mode - updated screenshots, tests, documentation

replaced <a> tag with tag

-removing redundant span container tag and moving fd-menu__item to li tag instead- for submenu though it has to have its parent menu item in its own span so that parent's states do not override submenu states -added role and tabindex to show tab order and focus added fd-fiori-focus() new function instead of defining in menu.scss- uses inner focus instead of outer focus -updated playground

- removed redundant right-arrow icons when submenu was not reeally present - instead used other icons to showcase secondary icon size - using separator in a different element - removed 'desktop' from modifier class name - removed transition property - applied font-size on icon directly - updated playground tests and screenshots accordingly - added selected-separated usecase to visual test

Renamed to fd-menu__separator modified playground test and documentation

removed comment from code added description and usage of classes to documentation removed hybrid usage of icons and long text from documentation, keeping it consistent, all these combinations are already preseent in playground added breaking changes to pr description

-added link class to aid implementation and support accessibility per aria standards. -added some aria attributes -modified playground test and documentation to support above changes

-also added is-selected mode and right/left arrow automatically on has-child class -using --submenu modifier on addon-after element will add the right arrow by default, if custom icon needs to be added, one can use it same as addon-before element without --submenu modifier. -updated playground tests and documentation to reflect these changes

kavya-b requested review from droshev, InnaAtanasova and JKMarkowski February 25, 2020 07:53

kavya-b force-pushed the feat/menu-fiori3-refactor branch from 4abe68e to 696fab1 Compare February 25, 2020 12:05

kavya-b added the Fiori 3 refactoring match Fiori 3 requirements label Feb 26, 2020

kavya-b self-assigned this Feb 26, 2020

kavya-b force-pushed the feat/menu-fiori3-refactor branch 2 times, most recently from 9557f63 to 345c251 Compare February 27, 2020 05:39

JKMarkowski suggested changes Feb 27, 2020

View reviewed changes

src/menu.scss Outdated Show resolved Hide resolved

src/menu.scss Outdated Show resolved Hide resolved

JKMarkowski suggested changes Feb 27, 2020

View reviewed changes

src/menu.scss Outdated Show resolved Hide resolved

src/menu.scss Outdated Show resolved Hide resolved

src/menu.scss Outdated Show resolved Hide resolved

src/menu.scss Outdated Show resolved Hide resolved

src/menu.scss Outdated Show resolved Hide resolved

rengare suggested changes Feb 27, 2020

View reviewed changes

kavya-b force-pushed the feat/menu-fiori3-refactor branch from 345c251 to c054f2f Compare February 28, 2020 03:23

droshev added the 0.8.0 label Feb 28, 2020

kavya-b force-pushed the feat/menu-fiori3-refactor branch from 3dffe1e to de2e4a9 Compare March 3, 2020 04:19

kavya-b force-pushed the feat/menu-fiori3-refactor branch 2 times, most recently from 299019b to 95240e2 Compare March 4, 2020 03:57

kavya-b requested review from JKMarkowski and rengare March 4, 2020 05:23

kavya-b force-pushed the feat/menu-fiori3-refactor branch from 95240e2 to 9bccd7f Compare March 5, 2020 08:10

kavya-b force-pushed the feat/menu-fiori3-refactor branch from a1dda42 to dc3be2e Compare March 9, 2020 08:01

rengare approved these changes Mar 9, 2020

View reviewed changes

JKMarkowski suggested changes Mar 9, 2020

View reviewed changes

kavya-b force-pushed the feat/menu-fiori3-refactor branch from d773b66 to 653d077 Compare March 24, 2020 12:08

droshev requested a review from Vanessa-Cusmich March 25, 2020 20:17

droshev self-requested a review March 27, 2020 01:43

droshev approved these changes Mar 27, 2020

View reviewed changes

kavya-b added this to PRs Review in progress in Development via automation Mar 27, 2020

Development automation moved this from PRs Review in progress to PRs Reviewer approved Mar 27, 2020

kavya-b and others added 12 commits March 27, 2020 09:01

feat: fiori3 menu refactor

46ad1ee

fix for underliine on <a> tag

5994ca6

replaced <a> tag with tag

disabled pointer events fix, updated visual tests for rtl

f169337

modify name of separator class in accordance with BEM methodology

325cc79

Renamed to fd-menu__separator modified playground test and documentation

fixed separator playground issue, added <a> tag and link classes

2b6b270

-added link class to aid implementation and support accessibility per aria standards. -added some aria attributes -modified playground test and documentation to support above changes

fix: make menu separator self-contained; update tests and doc

f5dc5cb

docs: limit the height of the dialogs rendered inline on the doc site

ee0c701

kavya-b force-pushed the feat/menu-fiori3-refactor branch from ad61bd3 to ee0c701 Compare March 27, 2020 03:32

droshev merged commit 1272053 into master Mar 29, 2020

Development automation moved this from PRs Reviewer approved to Done Mar 29, 2020

droshev deleted the feat/menu-fiori3-refactor branch March 29, 2020 02:07

meganaconley mentioned this pull request Apr 8, 2020

feat: update fundamental-styles to 0.8.2-rc.0 SAP/fundamental-react#961

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Menu refactor to Fiori3 #702

feat: Menu refactor to Fiori3 #702

kavya-b commented Feb 25, 2020 •

edited

netlify bot commented Feb 25, 2020 •

edited

JKMarkowski left a comment •

edited

JKMarkowski left a comment

rengare left a comment •

edited

kavya-b commented Feb 27, 2020 •

edited

The menu item has text decorator this doesn't match fiori 3

wrong width in IE 11

Could you add RTL example to documentation?

kavya-b commented Feb 27, 2020

rengare commented Feb 28, 2020

CLAassistant commented Mar 3, 2020

kavya-b commented Mar 4, 2020 •

edited

stefanoScalzo commented Mar 5, 2020

kavya-b commented Mar 5, 2020

JKMarkowski left a comment

JKMarkowski Mar 9, 2020

kavya-b Mar 10, 2020

droshev commented Mar 18, 2020

kavya-b commented Mar 19, 2020 •

edited

droshev commented Mar 22, 2020

kavya-b commented Mar 23, 2020

kavya-b commented Mar 26, 2020

droshev left a comment

feat: Menu refactor to Fiori3 #702

feat: Menu refactor to Fiori3 #702

Conversation

kavya-b commented Feb 25, 2020 • edited

Related Issue

Description

Breaking changes

Screenshots

Before:

After:

Updated screenshots:

netlify bot commented Feb 25, 2020 • edited

JKMarkowski left a comment • edited

Choose a reason for hiding this comment

JKMarkowski left a comment

Choose a reason for hiding this comment

rengare left a comment • edited

Choose a reason for hiding this comment

The menu item has text decorator this doesn't match fiori 3

wrong width in IE 11

Could you add RTL example to documentation?

kavya-b commented Feb 27, 2020 • edited

The menu item has text decorator this doesn't match fiori 3

wrong width in IE 11

Could you add RTL example to documentation?

kavya-b commented Feb 27, 2020

rengare commented Feb 28, 2020

CLAassistant commented Mar 3, 2020

kavya-b commented Mar 4, 2020 • edited

stefanoScalzo commented Mar 5, 2020

kavya-b commented Mar 5, 2020

JKMarkowski left a comment

Choose a reason for hiding this comment

JKMarkowski Mar 9, 2020

Choose a reason for hiding this comment

kavya-b Mar 10, 2020

Choose a reason for hiding this comment

droshev commented Mar 18, 2020

kavya-b commented Mar 19, 2020 • edited

droshev commented Mar 22, 2020

kavya-b commented Mar 23, 2020

kavya-b commented Mar 26, 2020

droshev left a comment

Choose a reason for hiding this comment

kavya-b commented Feb 25, 2020 •

edited

netlify bot commented Feb 25, 2020 •

edited

JKMarkowski left a comment •

edited

rengare left a comment •

edited

kavya-b commented Feb 27, 2020 •

edited

kavya-b commented Mar 4, 2020 •

edited

kavya-b commented Mar 19, 2020 •

edited