-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Use first non-empty Nav post as primary fallback for Nav block #36740
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.
Looks good so far.
I did a manual test by following the test instructions and did not see any issues. I did not do a code review. |
Addresses #36740 (comment)
Addresses #36740 (comment)
58ae500
to
2831ff3
Compare
Implements suggestion from #36740 (comment)
Co-authored-by: Adam Zielinski <adam@adamziel.com>
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.
I left a few suggestions related to naming, but otherwise this looks good to me! If you added a test or two, that would be absolutely stellar. Otherwise, 🚢
Co-authored-by: Adam Zielinski <adam@adamziel.com>
Let's ship and I'll follow up tomorrow with e2e tests. I'll also sort out the Page List block attribute vs context thing. |
@getdave I did s brief run through this before it was merged but didn't get to add my comments before it go merged. After this sequence:
I did see the only menu I had was rendered. Putting myself in the shoes of a site owner or builder. I saw the menu, saw it had the wrong pages in there, so I clicked on the Edit Site link on top of my screen. When the site editor loaded, I didn't know what to do next. My expectation would have been to see the menu now also in the editor, so I can make the changes, I wanted to make. I do like the default behavior. I just not sure what my expectation tells me about what happens next. Maybe this would work? Once the default menu is rendered on the frontend, because none was assigned before, that on subsequent Site editor views it could also be rendered there? Second observation: As if there is still an invisible navigation block. |
Thanks @bph - appreciate you taking time to test this. I'll double check we can still fallback to Page List and the behaviour around what happens if Page List isn't available. In terms of the UX we could show a warning on the block that it's currently showing a placeholder on the front end. I'm not confident we could land that in time for 5.9 though given everything else that's still outstanding. We've also received some feedback / concerns about the fallback situation. I'd like to see how that plays out during the Beta period. It's simple enough that, if we find the majority of folks are not onboard with that, we can revert to simply rendering nothing. Thanks again 🙇 |
* Use non-empty Nav post as fallback * Remove code duplication Addresses #36740 (comment) * Extract finding non empty nav block to function * Extract process of getting fallback to dedicated function * Apply block prefix to functions * Add additional safety check around parsing Nav blocks * Extract function for removing null blocks and apply * Check for empty parsed Nav blocks * Improve function comment Addresses #36740 (comment) * Amend ordering params to approximate current wp_nav_menus behaviour * Fetch Nav posts in query rather than filtering in memory Implements suggestion from #36740 (comment) * Improve comments * Spacing * Limit Nav Posts query to a single item Co-authored-by: Adam Zielinski <adam@adamziel.com> * Fix linting * Improve function naming Co-authored-by: Adam Zielinski <adam@adamziel.com> Co-authored-by: Adam Zielinski <adam@adamziel.com>
Description
In #36724 we added the Page List block as a fallback for when the Nav block has no menu selected.
This PR iterates on that idea by preferring to use the first non-empty Navigation (i.e.
wp_navigation
Post) as the primary fallback. Only if a suitable Navigation Post is not found do we fallback (again!) to the Page List.This aims to mirror how the fallback mechanic for
wp_nav_menu
works currently in Core (as described in the Issue).Closes #36721
How has this been tested?
Testing is as follows:
### Preparation
Appearance -> Navigation
).Testing
Screenshots
Types of changes
New feature (non-breaking change which adds functionality)
Checklist:
*.native.js
files for terms that need renaming or removal).