Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(platform): Adding initial code for new platform menu. #2117

Merged
merged 5 commits into from
Mar 11, 2020

Conversation

KevinOkamoto
Copy link
Member

@KevinOkamoto KevinOkamoto commented Mar 4, 2020

Please provide a link to the associated issue.

N/A

Please provide a brief summary of this pull request.

The new platform menu models itself after the Angular Material Menu. Instead of being one component "button" plus "dropdown menu", the "dropdown menu" is a separate component and can be attached/opened by any element using a "trigger" directive.

Please check whether the PR fulfills the following requirements

Documentation checklist:

Screen Shot 2020-03-04 at 1 34 51 PM

Screen Shot 2020-03-04 at 1 35 04 PM

The new platform menu models itself after the Angular Material Menu. Instead of being one component "button" plus "dropdown menu", the "dropdown menu" is a separate compoent and can be attached/opened by any element using a "trigger" directive.

Fixing lint issue

Adding new menu documentation

Updating Menu documentation

Adding menu in a scrollable container example

Updating menu examples

Adding aria-* attributes to menu trigger

Adding automatic generation of menu ID

transferring focus to child button
@netlify
Copy link

netlify bot commented Mar 4, 2020

Deploy preview for fundamental-ngx ready!

Built with commit ac326f5

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

@kavya-b
Copy link
Contributor

kavya-b commented Mar 5, 2020

  1. When navigating through mouse, on clicking a submenu item, the parent menu closes. But when navigating through keyboard, on clicking a submenu item, the parent menu does not close. Shouldn't this behaviour be consistent?
  2. Can we add an example that shows icons, separator lines and a longer menu with scrolling inside the menu; basically something like what the earlier 'complex menu' example had?

font-weight: 400;
font-size: 0.75rem;
text-align: center;
content: '\e066';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fiori team says although right arrow should be used to show a submenu, we shouldn't be restricting it to this particular icon in our code. @droshev could you please confirm if using it in this way is okay or should we provide developer the means to place his own icon instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. When navigating through mouse, on clicking a submenu item, the parent menu closes. But when navigating through keyboard, on clicking a submenu item, the parent menu does not close. Shouldn't this behaviour be consistent?

All parent menus should close when "mouse clicking" or "keyboard typing enter" on a leaf node.
I can't seem to reproduce the behavior you're describing.

  1. Can we add an example that shows icons, separator lines and a longer menu with scrolling inside the menu; basically something like what the earlier 'complex menu' example had?

I'll try and add the additional examples. Scrolling has not been implemented - I'll try to add that feature as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kavya-b I think I'm going to wait for the fundamental-styles update (0.8.x? the one with updated Menu styles) before adding examples with icons and separator lines. I'm guessing I'll have to redo the component templates when the update occurs.

Copy link
Member

@fkolar fkolar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My only comment but that probably the case for every single PR here is to get aligned on some formating I am putting private at the bottom, you at the top and we just need to enforce this with lint to keep it the same..

Looks good.

public click(): void {
this.itemClick.emit();

ngOnInit() { }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can probably remove this one as its not used..

@KevinOkamoto KevinOkamoto merged commit a3694ce into master Mar 11, 2020
@KevinOkamoto KevinOkamoto deleted the okamoto/new-platform-menu branch March 11, 2020 15:30
DeepakSap14 added a commit that referenced this pull request Mar 30, 2020
* feat(platform): Adding initial code for new platform menu.

The new platform menu models itself after the Angular Material Menu. Instead of being one component "button" plus "dropdown menu", the "dropdown menu" is a separate compoent and can be attached/opened by any element using a "trigger" directive.

Fixing lint issue

Adding new menu documentation

Updating Menu documentation

Adding menu in a scrollable container example

Updating menu examples

Adding aria-* attributes to menu trigger

Adding automatic generation of menu ID

transferring focus to child button

* Removing focus handler

* Fixing unit tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants