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

Curator: add mobile menu styling #6146

Merged
merged 4 commits into from
Jul 5, 2022
Merged

Conversation

mikachan
Copy link
Member

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:

image

Related issue(s):

Closes #6095

@mikachan mikachan added the [Theme] Vivre Automatically generated label for Vivre. label Jun 28, 2022
@mikachan mikachan added this to the Curator milestone Jun 28, 2022
@mikachan mikachan requested a review from a team June 28, 2022 19:33
@mikachan mikachan self-assigned this Jun 28, 2022
Copy link
Contributor

@jffng jffng left a 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;
Copy link
Contributor

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.

Copy link
Member Author

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).

Copy link
Contributor

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.

Copy link
Member Author

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.

curator/theme.json Outdated Show resolved Hide resolved
Copy link
Contributor

@pbking pbking left a 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.

🚢

@pbking pbking merged commit 3ffb927 into trunk Jul 5, 2022
@mikachan mikachan deleted the add/curator-mobile-menu-styling branch July 6, 2022 08:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Theme] Vivre Automatically generated label for Vivre.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Curator: mobile menu styling
3 participants