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

Videomaker: Navigation styles #4876

Merged
merged 13 commits into from
Oct 20, 2021
Merged

Videomaker: Navigation styles #4876

merged 13 commits into from
Oct 20, 2021

Conversation

mikachan
Copy link
Member

@mikachan mikachan commented Oct 20, 2021

Changes proposed in this Pull Request:

This PR tweaks the Videomaker navigation styles to match the Figma designs.

Before After
image image

It looks like GB doesn't pull through the current-menu-item class, so we're not able to style the active menu item differently at the moment. I've made a change to the navigation attributes PR on GB that fixes this, so it's possible to test on this branch: WordPress/gutenberg#35634.

Without the GB PR, I think we'll need to remove the submenu styles. At the moment I've hidden all child menu items unless the parent is the current menu item, which means they'll be hidden by default without the current-menu-item class. Let me know if you want me to remove these now - or maybe comment them out while we wait for a fix?

I've made sure the menus in the patterns are still uppercase by adding an inline style to the nav blocks in the patterns.

I've also only styled the responsive menu, as it looks like Videomaker should always use the responsive menu when this can be merged: #4832

Related issue(s):

Closes #4651

@mikachan mikachan added the [Theme] Videomaker Automatically generated label for videomaker. label Oct 20, 2021
@mikachan mikachan self-assigned this Oct 20, 2021
@mikachan mikachan marked this pull request as ready for review October 20, 2021 10:26
@mikachan mikachan requested a review from a team October 20, 2021 10:27
@@ -3,6 +3,6 @@
<!-- wp:site-logo /-->
<!-- wp:site-title /-->
<!-- wp:site-tagline {"style":{"typography":{"fontSize":"var(--wp--custom--font-sizes--tiny)"}}} /-->
<!-- wp:navigation {"itemsJustification":"right","isResponsive":true,"__unstableLocation":"primary","__unstableSocialLinks":"social"} /-->
<!-- wp:navigation {"className":"is-style-blockbase-navigation-improved-responsive","itemsJustification":"right","isResponsive":true,"__unstableLocation":"primary","__unstableSocialLinks":"social"} /-->
Copy link
Member

Choose a reason for hiding this comment

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

I added this for testing, but I do think it looks better too. What do you think @kjellr ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I think this can use the standard block style. 👍

@scruffian scruffian merged commit 46f1ed1 into trunk Oct 20, 2021
@scruffian scruffian deleted the fix/videomaker-nav-styles branch October 20, 2021 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Theme] Videomaker Automatically generated label for videomaker.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

videomaker: Templates: Header template part (including mobile)
3 participants