-
Notifications
You must be signed in to change notification settings - Fork 6
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
Update local header layout #67
Conversation
fbe0e87
to
90a8891
Compare
source/wp-content/themes/wporg-documentation-2022/parts/header-home.html
Outdated
Show resolved
Hide resolved
It looks great. I just noticed one thing: "Technical guides" link needs to be in the active style (bold font-weight). On the other hand, I checked the mockups and the Article design misses the breadcrumb. The local header does need to show the breadcrumb when you land in article posts ("Block Directory" in this example). |
source/wp-content/themes/wporg-documentation-2022/parts/header-navigation.html
Outdated
Show resolved
Hide resolved
source/wp-content/themes/wporg-documentation-2022/patterns/header.php
Outdated
Show resolved
Hide resolved
source/wp-content/themes/wporg-documentation-2022/parts/header-home.html
Outdated
Show resolved
Hide resolved
Updated Screenshots with local nav bar and dynamic nav menu from mu-plugins Home (with edited content)
Topic landing page
|
/** | ||
* Provide a list of local navigation menus. | ||
*/ | ||
function add_site_navigation_menus( $menus ) { |
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.
Local navigation menus now defined here, rather than in the mu-plugins block
b869495
to
caee61e
Compare
Awaiting confirmation before shipping. |
Francisco is AFK for the moment, so I can only be a silver-medal in terms of feedback. Notably, I would be okay with shipping this if you all feel it's a strong path forward and not too code-complex. However from gleaning the navigation component that Francisco mocked up here, it isn't clear to me that we are employing it right. From the WP.org design library:
News component, closed: Opened: It can be a modal if need be, we can work with that, but it would be good to get the chevron icon right instead of the "Menu" label at least. |
@jasmussen If you want to see how this currently works, it's already in place on the about & download subpages, for example on About > Requirements. The menu could also be on a dark background like on the Download Counter.
In the original local header discussion (it's long, relevant comments start here), I shared that the label for the menu should ideally stay consistent throughout the individual site. I think we landed on "menu" by default, but could pick something else. In any case, since this is a shared component, changing the functionality would need to happen in wporg-mu-plugins. So I'm inclined to let this PR through, and have any follow up design changes to a new issue on that repo. |
- Move breadcrumbs below nav bar
Color now added to parent theme is white-opacity-15
caee61e
to
370da8b
Compare
I'm happy to ship and iterate on this one, though I do want to surface that removing the breadcrumbs on one of the subpages was a mistake on my part in the mockups. I may owe an apology. |
Opened a new issue for this |
Closes WordPress/wporg-main-2022#66
Depends on
WordPress/wporg-parent-2021#103,WordPress/wporg-mu-plugins#443and WordPress/wporg-mu-plugins#439Home header background color can be updated in content.
Screenshots (outdated, see below)
Home
Topic landing page
Article
How to test the changes in this Pull Request:
on the branches detailed above