Navigation block: add location->primary to fallback nav creation for classic menus. #45976
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
scruffian
reviewed
Nov 23, 2022
|
|
||
| // Attempts to return the menu assigned to location `primary` (a convention many classic themes follow). | ||
| $locations = get_nav_menu_locations(); | ||
| if ( isset( $locations[ 'primary' ] ) ) { |
There was a problem hiding this comment.
Do we need to check whether $locations is not empty?
There was a problem hiding this comment.
The function returns an empty array if they are false.
Calling isset on a key in empty array should result in false.
I suppose a simple test would be to remove all your classic menus and run the code.
There was a problem hiding this comment.
Calling isset on a key in empty array should result in false.
👍
I suppose a simple test would be to remove all your classic menus and run the code.
The test I ran to verify was switching to a block theme the following conditions:
- the block theme has no menu locations defined
- no classic menus defined
- no published navigation menus
- viewing the front-end and verifying no errors
|
This looks like a good start. We'll need to make the same changes to the editor, so that the fallbacks in the editor match those on the frontend. |
getdave
reviewed
Nov 23, 2022
@jffng We have an entity for Request to Try running this in the console on the Post editor screen when you have classic menus: |
👍 It occurred to me — this added fallback is only relevant for an active theme with a So if I switch from a theme with a primary menu location to a block theme with no primary menu location, this fallback won't apply. Do you still think it's worth it? |
|
Size Change: +56 B (0%) Total Size: 1.32 MB
ℹ️ View Unchanged
|
Hmm. Is there any way to work out what was on primary in the previous theme? Could we just use a menu called primary if one exists? |
Yes, we could add this condition. Done in a5a5001 |
getdave
reviewed
Nov 29, 2022
getdave
reviewed
Dec 1, 2022
getdave
reviewed
Dec 1, 2022
getdave
reviewed
Dec 1, 2022
getdave
reviewed
Dec 1, 2022
getdave
reviewed
Dec 1, 2022
scruffian
approved these changes
Dec 1, 2022
scruffian
force-pushed
the
update/nav-block-classic-menu-fallback
branch
from
9bc3bd4 to
f7fe5b1
Compare
|
Playwright tests are failing but it appears to be across the repo. |
|
Tests passing, going to merge this but please let me know if there are any follow ups / if I misunderstood any of the most recent feedback. |
getdave
added
the
Needs Dev Note
|
This will need documenting come WP 6.2 so I added the label. |
mpkelly
pushed a commit
to mpkelly/gutenberg
that referenced
this pull request
Dec 7, 2022
…classic menus. (WordPress#45976) * Add location primary to classic menu fallback creation. * Fix accidental line removal. * Request and export entities for menuLocations. * Check for primary menu during classic menu conversion. * Make linter happy. * Check for primary menu location OR slug. * Remove ununused hooks. * Fix php lint errors. * Look for menu with slug named primary.
|
@jffng As you created this PR would you like to write the dev note? Happy to do it but wondered if you'd prefer to. |
|
I'd be happy to write it, thanks for the ping. |
Dev Note for 6.2cc @bph Retaining navigation when switching themes If a site has a classic menu, and has not yet created a navigation using the navigation block, then the navigation block will automatically import a classic menu to use. Many classic themes (for example TT1) use the primary name and slug as a convention for its main menu location. If a site has a classic menu defined at this location, then the navigation block will import this one. If not then it will use the most recently created classic menu. The conditions for deciding which classic menu to import are:
If these conditions are met, the navigation block will display the classic menu, automatically converting it to navigation menu. |
|
@scruffian yeah that's better because there's more context to how fallbacks work overall. I'll update your comment and we can go with that. Thanks! |
bph
added
has dev note
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
This PR adds a check for a classic menu whose location is set to
primary, to the series of fallbacks.Why?
Many themes (for example TT1) use
primaryslug as a convention for the location for the main classic menu, so this is slightly more friendly to upgrade from a classic theme / menu if theprimarylocation is assigned.How?
See above.
Testing Instructions
primarylocation defined eg Zoologist and create + assign a menu to that location.Screenshots or screencast