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

Menus should add padding #708

Closed
WordlessEcho opened this issue May 24, 2017 · 6 comments · Fixed by #718
Closed

Menus should add padding #708

WordlessEcho opened this issue May 24, 2017 · 6 comments · Fixed by #718
Assignees

Comments

@WordlessEcho
Copy link

In Material Design Guidelines, menus should have:

  • Top padding: 8dp
  • Bottom padding: 8dp

components_menus_specs1.png (1520×768)

But Material Design Components Web Menu only have bottom padding, have no top padding:

Simple Menu - Material Components Catalog

Anyway, seems that menus "dividing line" should have 16px padding? Guidelines is not clear about that.

components_menus_specs4.png (1520×1412)

@robzenn92
Copy link
Contributor

robzenn92 commented May 24, 2017

@WordlessEcho nice catch! However, to me it seems that it does not have the bottom padding too. To the best of my knowledge the little space between the last child and the end of the menu is caused by line 66.

Isn't enough to remove line 66 and replace line 128 with padding: 8px 0; ? Am I wrong?

Regarding the dividing line I think it definitely needs to be adjusted.

EDIT: by the way now I see that there is a difference between mobile and desktop: so on mobile the top-bottom padding must be 8px while on tablet/desktop it needs to be 16px.

@WordlessEcho
Copy link
Author

Yep! That's right!

right_menus

@robzenn92
Copy link
Contributor

robzenn92 commented May 25, 2017

Cool! 😃 So I am going to open a PR with the fix!

There is still the need to understand whether the desktop should have a 16px padding and where to put it, cause I don't see media queries. Hence, I think that the whole desktop style is missing.

@WordlessEcho
Copy link
Author

Top padding, bottom padding and dividing line need 8px on mobile, 16px on desktop (Anyway, seems that table is same of desktop).
Thanks!

@robzenn92
Copy link
Contributor

Actually, here I found the following:

Default desktop menu
Menu item height: 32px
Menu item text left padding: 24px
Top padding: 8px
Bottom padding: 8px
Typography: 15px

So both the top and bottom padding for desktop are the same as the mobile while the difference is the Menu item height and the Menu item text left padding.

The third picture you posted for the dividing line is related to the Cascading menu which is different. See #53.

@WordlessEcho
Copy link
Author

The third picture you posted for the dividing line is related to the Cascading menu which is different.

I nearly forget it... You're right. XD

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 a pull request may close this issue.

3 participants