-
Notifications
You must be signed in to change notification settings - Fork 360
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking this on! I left two comments, but in general the mobile menu is looking good to me.
*/ | ||
.wp-block-navigation__responsive-container.is-menu-open ul { | ||
font-size: var(--wp--preset--font-size--huge); | ||
font-weight: 300; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The design suggests the items are also vertically centered, but I don't think we should add CSS for that here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah OK.
It would only be one line of CSS, but I know any additional CSS isn't great...
.wp-block-navigation__responsive-container.is-menu-open {
justify-content: center;
}
This could be an option in GB, and it probably fits into the other conversations around adding styling controls to the mobile navigation (e.g. WordPress/gutenberg#39142).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would only be one line of CSS ...
That would only work for menus that fit completely in the window. Otherwise you can't scroll to get to what's "above" halfway.
There are ways to do it (I think we achieved it with Videomaker eventually) but it's not the simplest of solutions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would only work for menus that fit completely in the window. Otherwise you can't scroll to get to what's "above" halfway.
Ahh ok, that's not a great solution then!
If it becomes a problem we can add the CSS then, but it sounds like it's best to leave this for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me and seems to match the design.
🚢
Changes proposed in this Pull Request:
This PR adds styling for the mobile navigation in Curator. I worked off the styles in Figma but let me know if anything doesn't look right:
Related issue(s):
Closes #6095