-
Notifications
You must be signed in to change notification settings - Fork 30
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
Try new Local Navigation block #269
Conversation
Thanks a bunch, this looks great. I don't know for sure that we will always have the borders visible, and depending on context (bg color etc), they might need to be inverted or semi-transparent. I'm not sharing that as blocking feedback, it's clear you're already handling on a case by case basis with the Download section. But it is to say, I would love to get some feedback from @panchovm when he gets a moment, notably around this one: For example, Should it say "About WordPress.org" instead of "About" or "WordPress.org"? And should the submenu item match? I think Francisco may be back from some AFK tomorrow and can sanity check. |
@jasmussen @fcoveram Do you think this can be merged, or do you have more feedback? The Local Navigation container block itself has already been merged, so the main issue here is whether the design works. |
I'm happy to iterate this, we can always revisit. I'll defer to Francisco in case he chimes in. |
Sorry for jumping in late here. I checked the images and realized we did not explain the header correctly. I took some pages of Learn site to show how the header behaves at each depth level. See the diagram below Per level depth:
Showing the breadcrumb at level 2 is redundant as it will show the home and active page links. The border dividing header and breadcrumb is not always visible but depends on the subsite style. But as Joen said, it shouldn't block the ticket. |
Thanks for the explanation! I'm pretty sure this PR only changes the "level 3" pages, so I think I've got it right? With the exception of the Download Counter.
So does the breadcrumb need to update to be Home / About / [current page]? Or was there other feedback about these specific pages? |
f76f12d
to
4fa998d
Compare
I missed a crucial point in my explanation, WordPress.org homepage is not counted as L1. In the example attached, the subsite's homepage (Learn homepage) is L1. So following the site structure you shared above, the logic I suggested goes as follows:
What's currently in production works well because L1 (About) and L2 (Requirements, Features, Security, etc) don't show the breadcrumb but active style in the subnavigation area.
In the "About" case, my comment quoted above means that "home" link in breadcrumb takes you to About's homepage. |
6274a50
to
177e61a
Compare
Ah right— we got caught up in a technical-vs-conceptual issue 🙂 technically, wordpress.org and learn.wordpress.org are the same level in each site (both roots/homepages), and About is a subpage in WordPress.org— but conceptually, you're thinking of About and Download as unique "sites" … or maybe it's more correct to say that the WordPress.org homepage itself is the special case? So would a page like Enterprise or Mobile be L1, even though they have no child pages? In any case, the updated PR now only shows the section title (About or Download) and the navigation, for each page. Like this: |
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.
👍 LGTM
To @ryelle
Yes, yes, and yes 😅 . I understand it's technically wrong, but the approach was taken for browsing purposes.
And it looks great ✨ 🚀 |
177e61a
to
517ea2f
Compare
The breadcrumbs were removed from subpages before #269 was merged, but this configuration code was left in place. It's not used, and can be removed
This is a companion PR to WordPress/wporg-mu-plugins#404, which introduces a new Local Navigation container to hold the section title & section menu. This PR extracts out the local header from the synced post content, and creates new reusable pattern-partials which handle the header content on each subpage. The new patterns use the Local Navigation container block.
The changes here also bring these headers in line with the generic section header mocked up. @WordPress/meta-design As I understood it, this pattern is complete, but if that's not the case let me know.
❗ If we decide not to merge the Local Nav block, I still think we should merge e1e42a3 to DRY up the About pages. That won't change any visuals.
Additionally, once this is merged, I'll need to go in and update each of the subpages to remove the banner, so that the content sync doesn't overwrite these changes.
See WordPress/wporg-mu-plugins#465
Screenshots
How to test the changes in this Pull Request: