feat(platform): Menu as a standalone component #1390

kavya-b · 2019-10-11T11:26:01Z

Please provide a link to the associated issue.

Please provide a brief summary of this pull request.

This PR contains code for development of Menu as a standalone component, with varying levels of customization. The design is documented here: https://github.com/SAP/fundamental-ngx/wiki/Platform:-Menu-component-Technical-Design

If this is a new feature, have you updated the documentation?

This is a new feature; documentation is pending.

netlify · 2019-10-11T11:30:46Z

Deploy preview for fundamental-ngx ready!

Built with commit 398db1d

https://deploy-preview-1390--fundamental-ngx.netlify.com

libs/platform/src/lib/components/menu/menu-component/menu-item.component.html

libs/platform/src/lib/components/menu/menu-component/menu-item.component.scss

fkolar · 2019-10-15T08:57:31Z

Can you please try to do rebase? we shoudl not see any Merge messgage in the commit

kavya-b · 2019-10-15T09:02:54Z

Can you please try to do rebase? we should not see any Merge messgage in the commit

Oops, forgot to do it this time. Currently branch is up-to-date with master, so it is not allowing me to rebase, Frank. I will do rebase as soon as someone pushes changes to ngx next time.

kavya-b · 2019-10-16T12:28:52Z

Can you please try to do rebase? we shoudl not see any Merge messgage in the commit

done.

libs/platform/src/lib/components/menu/menu.module.ts

fkolar · 2019-10-21T08:01:13Z

In general besides other comments we should follow the same path as core import scss from fundamental-style, and dont touch anything that is already define once, otherwise we will break theming.

When working with styles and in the component we have set viewencapsulation.none, lets be aware that it means Don't provide any template or style encapsulation, in other words that style from your menu-item scss will be put on top of the html file available for everybody and changing everything that other components sets (using the same styles).

kavya-b · 2019-10-21T08:41:31Z

In general besides other comments we should follow the same path as core import scss from fundamental-style, and dont touch anything that is already define once, otherwise we will break theming.

When working with styles and in the component we have set viewencapsulation.none, lets be aware that it means Don't provide any template or style encapsulation, in other words that style from your menu-item scss will be put on top of the html file available for everybody and changing everything that other components sets (using the same styles).

There is a huge gap between what our invision spec says and what fundamental-styles currently provides, Frank. I had already raised corresponding issues on fundamental-styles for this gap (styles Menu open issues) but only one has been addressed till now.

So, for now, what should be the way to proceed? Should I be dropping all css changes entirely?; this will break some unit and automation tests in addition to the entire look of the Menu component.

KevinOkamoto · 2019-10-21T14:46:16Z

For most of our components, we should be trying to use the "core" directives and components in our templates - so we should be getting the styling automatically.

If there are gaps between our design and what fundamental-styles provides, then yes we need to add customized CSS rules to our components. We do need to be aware of theming however and try to use the CSS variable definitions of fundamental-styles, so that our color palate is the same.

kavya-b · 2019-10-22T02:35:55Z

For most of our components, we should be trying to use the "core" directives and components in our templates - so we should be getting the styling automatically.

If there are gaps between our design and what fundamental-styles provides, then yes we need to add customized CSS rules to our components. We do need to be aware of theming however and try to use the CSS variable definitions of fundamental-styles, so that our color palate is the same.

The "core" directives in Menu are mostly binding to the fundamental-styles classes such as fd-menu__item, fd-menu__group, fd-menu--addon-before, fd-menu__list etc. I am using these same classes directly in "platform" and wherever gaps are present, I am overriding them. I am also using the same CSS variable definitions wherever I can.

However, I am also adding additional classes for secondaryIcon such as fd-menu--addon-after which do not exist in fundamental-styles at all currently. Theming then could affect secondaryIcon and we should probably get fundamental-styles to also implement a secondaryIcon feature for consistency.

kavya-b · 2019-10-23T06:14:50Z

@fkolar @KevinOkamoto changed name from MenuModule to FdpMenuModule.

fkolar · 2019-10-24T12:05:06Z

For most of our components, we should be trying to use the "core" directives and components in our templates - so we should be getting the styling automatically.
If there are gaps between our design and what fundamental-styles provides, then yes we need to add customized CSS rules to our components. We do need to be aware of theming however and try to use the CSS variable definitions of fundamental-styles, so that our color palate is the same.

The "core" directives in Menu are mostly binding to the fundamental-styles classes such as fd-menu__item, fd-menu__group, fd-menu--addon-before, fd-menu__list etc. I am using these same classes directly in "platform" and wherever gaps are present, I am overriding them. I am also using the same CSS variable definitions wherever I can.

However, I am also adding additional classes for secondaryIcon such as fd-menu--addon-after which do not exist in fundamental-styles at all currently. Theming then could affect secondaryIcon and we should probably get fundamental-styles to also implement a secondaryIcon feature for consistency.

Even if they are just directive that adds class, we should still use them. The reason is that once styles do some new adjustments and some renamign, you dont want to change this on two places. Let;'s use core directives as wrapper for the target class. . YOu dont want to be using directly class if some other component is wrapping them.

kavya-b · 2019-10-25T06:56:11Z

For most of our components, we should be trying to use the "core" directives and components in our templates - so we should be getting the styling automatically.
If there are gaps between our design and what fundamental-styles provides, then yes we need to add customized CSS rules to our components. We do need to be aware of theming however and try to use the CSS variable definitions of fundamental-styles, so that our color palate is the same.

The "core" directives in Menu are mostly binding to the fundamental-styles classes such as fd-menu__item, fd-menu__group, fd-menu--addon-before, fd-menu__list etc. I am using these same classes directly in "platform" and wherever gaps are present, I am overriding them. I am also using the same CSS variable definitions wherever I can.
However, I am also adding additional classes for secondaryIcon such as fd-menu--addon-after which do not exist in fundamental-styles at all currently. Theming then could affect secondaryIcon and we should probably get fundamental-styles to also implement a secondaryIcon feature for consistency.

Even if they are just directive that adds class, we should still use them. The reason is that once styles do some new adjustments and some renamign, you dont want to change this on two places. Let;'s use core directives as wrapper for the target class. . YOu dont want to be using directly class if some other component is wrapping them.

Okay, yes, you're right. Made the changes accordingly. Please check.

droshev · 2019-10-25T11:15:29Z

@kavya-b if you think the colors are not correct I would suggest to do a fix in styles instead of adding custom css here

kavya-b · 2019-10-25T11:52:58Z

@kavya-b if you think the colors are not correct I would suggest to do a fix in styles instead of adding custom css here

Hi Deno, are you suggesting to do this for only colors or any/all styling? What about adding new classes such as for a secondary icon?

Removed custom elements, added some css aligning with spec, made a MenuModule for using with demo app

- added scrolling and scrollingLimit attributes - added some CSS to align with spec - modified existing unit tests - added optional separator line

such as: disabled, tooltip customization, keyboard accessibility, var colors to scss, fixed compiler error issue

also, rtl fixes for icon and text secondary icon css fix truncation with inline-flex issue resolved

Additionally, 1. scrollbar styling aligned with specs 2. state styling issues fixed 3. added id and childItems to MenuItem interface 4. added/modified unit tests 5. handled secondaryIcon

Removed custom elements, added some css aligning with spec, made a MenuModule for using with demo app

- added scrolling and scrollingLimit attributes - added some CSS to align with spec - modified existing unit tests - added optional separator line

such as: disabled, tooltip customization, keyboard accessibility, var colors to scss, fixed compiler error issue

also, rtl fixes for icon and text secondary icon css fix truncation with inline-flex issue resolved

Additionally, 1. scrollbar styling aligned with specs 2. state styling issues fixed 3. added id and childItems to MenuItem interface 4. added/modified unit tests 5. handled secondaryIcon

Removing Highlightable and active classes, renaming customLabel to tooltipLabel

@ViewChild

-fixed usage of method call in template as suggested -added hardcoded color values to support IE11 -fixed failing testcases due to ang8 update not detecting @ViewChild properly

…yles added MenuModule back into platform.module.ts after it got removed while merging conflicts

also provided fix for handling width in em or px and invalid width values.

fkolar · 2019-10-30T18:12:04Z

Anything new on this PR ?

kavya-b · 2019-10-31T02:43:30Z

Anything new on this PR ?

nothing new added, only rebased multiple times

kavya-b added the platform platform label Oct 11, 2019

kavya-b requested review from fkolar and KevinOkamoto October 11, 2019 11:26

kavya-b self-assigned this Oct 11, 2019

KevinOkamoto suggested changes Oct 11, 2019

View reviewed changes

libs/platform/src/lib/components/menu/menu-component/menu-item.component.html Outdated Show resolved Hide resolved

libs/platform/src/lib/components/menu/menu-component/menu-item.component.scss Outdated Show resolved Hide resolved

KevinOkamoto added this to In Review in Platform Development via automation Oct 11, 2019

kavya-b force-pushed the feat-menu-component branch from ce32f9f to b2dffba Compare October 16, 2019 09:56

kavya-b requested a review from KevinOkamoto October 17, 2019 10:34

KevinOkamoto reviewed Oct 17, 2019

View reviewed changes

libs/platform/src/lib/components/menu/menu.module.ts Outdated Show resolved Hide resolved

kavya-b force-pushed the feat-menu-component branch 2 times, most recently from b21bdb7 to 52c84f2 Compare October 21, 2019 03:18

fkolar requested a review from KevinOkamoto October 21, 2019 08:01

kavya-b force-pushed the feat-menu-component branch 2 times, most recently from b113cbd to e20077b Compare October 23, 2019 02:59

kavya-b force-pushed the feat-menu-component branch from 423e90e to cbba981 Compare October 24, 2019 05:27

kavya-b force-pushed the feat-menu-component branch from cbba981 to c7aa290 Compare October 25, 2019 06:03

kavya-b force-pushed the feat-menu-component branch from b9ccc94 to e209328 Compare October 25, 2019 12:18

kavya-b force-pushed the feat-menu-component branch from e209328 to 4d6740d Compare October 28, 2019 02:23

kavya-b added 20 commits October 30, 2019 08:14

Integrating Kevin's af-menu from shellbar to ngx

9df2e5e

Removed custom elements, added some css aligning with spec, made a MenuModule for using with demo app

Fixing some build errors

dee3769

feat: Issue #1264 Development of menu component

cbb3f0e

- added scrolling and scrollingLimit attributes - added some CSS to align with spec - modified existing unit tests - added optional separator line

feat(platform): #1264 added support for attributes

b6cf8fb

such as: disabled, tooltip customization, keyboard accessibility, var colors to scss, fixed compiler error issue

feat(platform): #1264 fixed optional separator css issue

3420c89

feat(platform): #1264 css changes, code reorganized into proper folder

4cfe9cf

also, rtl fixes for icon and text secondary icon css fix truncation with inline-flex issue resolved

feat(platform): using KeyboardService from core for accessibility

344671c

Additionally, 1. scrollbar styling aligned with specs 2. state styling issues fixed 3. added id and childItems to MenuItem interface 4. added/modified unit tests 5. handled secondaryIcon

Integrating Kevin's af-menu from shellbar to ngx

f1582e9

Removed custom elements, added some css aligning with spec, made a MenuModule for using with demo app

Fixing some build errors

4efe49b

feat: Issue #1264 Development of menu component

6ddc555

- added scrolling and scrollingLimit attributes - added some CSS to align with spec - modified existing unit tests - added optional separator line

feat(platform): #1264 added support for attributes

ceff64f

such as: disabled, tooltip customization, keyboard accessibility, var colors to scss, fixed compiler error issue

feat(platform): #1264 fixed optional separator css issue

b7f7564

feat(platform): #1264 css changes, code reorganized into proper folder

6af938f

also, rtl fixes for icon and text secondary icon css fix truncation with inline-flex issue resolved

feat(platform): using KeyboardService from core for accessibility

832a9f6

Additionally, 1. scrollbar styling aligned with specs 2. state styling issues fixed 3. added id and childItems to MenuItem interface 4. added/modified unit tests 5. handled secondaryIcon

style(platform): cleaning up code

d890656

Removing Highlightable and active classes, renaming customLabel to tooltipLabel

style(platform): cleaned up code

e9c7239

fix(platform): addressed review comment changes

9b064a8

-fixed usage of method call in template as suggested -added hardcoded color values to support IE11 -fixed failing testcases due to ang8 update not detecting @ViewChild properly

style(platform): removed an incorrect color value from fundamental st…

447ac76

…yles added MenuModule back into platform.module.ts after it got removed while merging conflicts

fix(platform):changed name from MenuModule to FdpMenuModule

0804e04

fix(platform): reused core directives

398db1d

also provided fix for handling width in em or px and invalid width values.

kavya-b force-pushed the feat-menu-component branch from 4d6740d to 398db1d Compare October 30, 2019 02:44

Platform Development automation moved this from In Review to Approved by Reviewer Oct 30, 2019

KevinOkamoto approved these changes Oct 30, 2019

View reviewed changes

kavya-b merged commit 99ea344 into master Oct 31, 2019

droshev deleted the feat-menu-component branch November 17, 2019 19:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(platform): Menu as a standalone component #1390

feat(platform): Menu as a standalone component #1390

kavya-b commented Oct 11, 2019

netlify bot commented Oct 11, 2019 •

edited

fkolar commented Oct 15, 2019

kavya-b commented Oct 15, 2019

kavya-b commented Oct 16, 2019

fkolar commented Oct 21, 2019

kavya-b commented Oct 21, 2019

KevinOkamoto commented Oct 21, 2019

kavya-b commented Oct 22, 2019

kavya-b commented Oct 23, 2019

fkolar commented Oct 24, 2019

kavya-b commented Oct 25, 2019

droshev commented Oct 25, 2019

kavya-b commented Oct 25, 2019

fkolar commented Oct 30, 2019

kavya-b commented Oct 31, 2019

feat(platform): Menu as a standalone component #1390

feat(platform): Menu as a standalone component #1390

Conversation

kavya-b commented Oct 11, 2019

Please provide a link to the associated issue.

Please provide a brief summary of this pull request.

If this is a new feature, have you updated the documentation?

netlify bot commented Oct 11, 2019 • edited

fkolar commented Oct 15, 2019

kavya-b commented Oct 15, 2019

kavya-b commented Oct 16, 2019

fkolar commented Oct 21, 2019

kavya-b commented Oct 21, 2019

KevinOkamoto commented Oct 21, 2019

kavya-b commented Oct 22, 2019

kavya-b commented Oct 23, 2019

fkolar commented Oct 24, 2019

kavya-b commented Oct 25, 2019

droshev commented Oct 25, 2019

kavya-b commented Oct 25, 2019

fkolar commented Oct 30, 2019

kavya-b commented Oct 31, 2019

netlify bot commented Oct 11, 2019 •

edited