Navigation: Fallback to a classic menu if one is available #44173
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
requested review from
draganescu,
getdave,
talldan and
cbravobernal
|
Size Change: +3.02 kB (0%) Total Size: 1.27 MB
ℹ️ View Unchanged
|
I don't think this is a problem, as the classic menu would already be showing in the frontend, as the conditions are the same:
|
cbravobernal
changed the title
I completely agree with this too 😄 |
|
I think we should try and only address the most basic use case:
If the website has multiple classic menus we should not do anything. We can come back later with some implementation that matches classic "location" to block template part "slug", but I think this would be more complex. |
|
I've made some changes to this. Now, if there's no wp_navigation CPTs and one classic menu, we convert the classic menu to a wp_navigation CPT, and this becomes the fallback. This happens in both the editor and the frontend. |
draganescu
requested changes
Sep 16, 2022
There was a problem hiding this comment.
Awesome! The code looks almost there, I left some tweaks. Requesting changes mostly about making sure we maintain the fallback behavior of not dirtying the post on fallback.
Did not test visually yet.
Later edit:, visual testing results:
- The template part ends up dirty upon fallback. As mentioned in code, we should not assign the ref, but allow the fallback to set it marked as
__unstableMarkNextChangeAsNotPersistent. - The import happens multiple times:
Screenshot - Initial load
Screenshot - Second refresh
- The menu is imported as
draft. Considering this is the only classic menu that exists it's fine to import it aspublished.
All menus are draft
| } | ||
| }; | ||
|
|
||
| onSelectClassicMenu( classicMenus[ 0 ] ); |
There was a problem hiding this comment.
I think here we should use the same process that we use for the block menus, that is avoid calling handleUpdateMenu. This call sets the ref and the use sees a dirty post. We should instead just create the block menu, then because it's the latest created it will be handled by the later code.
packages/block-library/src/navigation/edit/use-convert-classic-menu-to-block-menu.js
Show resolved
Hide resolved
|
Thanks for testing @draganescu, I have addressed all the feedback here, so it's ready for another look. |
draganescu
approved these changes
Sep 19, 2022
There was a problem hiding this comment.
Tested this again after e5f8fb5 and it worked pretty smooth:
- one classic menu resulted in auto importing it and the block theme's empty navigation block used it as a fallback.
scruffian
requested review from
spacedmonkey and
tellthemachines
as code owners
draganescu
previously requested changes
Sep 19, 2022
getdave
reviewed
Sep 20, 2022
getdave
reviewed
Sep 21, 2022
draganescu
force-pushed
the
update/navigation-classic
branch
from
3c4e9ea to
5fcf224
Compare
getdave
reviewed
Sep 21, 2022
getdave
reviewed
Sep 22, 2022
|
I found a bug with this. When you have two navigation blocks on the page they will both try to create a classic navigation, and do so successfully. |
Co-authored-by: Dave Smith <getdavemail@gmail.com>
Co-authored-by: Dave Smith <getdavemail@gmail.com>
draganescu
force-pushed
the
update/navigation-classic
branch
from
77dafd0 to
6249285
Compare
|
@draganescu and I created a solution to prevent menus from importing twice. We'd be grateful for a review. We tried creating a store and then saving state of the import in the store, but the store didn't seem to update fast enough so we still experienced the problem of the import happening twice. This module level variable seems to solve it though... |
getdave
reviewed
Oct 5, 2022
packages/block-library/src/navigation/edit/use-convert-classic-menu-to-block-menu.js
Outdated
Show resolved
Hide resolved
packages/block-library/src/navigation/edit/use-convert-classic-menu-to-block-menu.js
Outdated
Show resolved
Hide resolved
packages/block-library/src/navigation/edit/use-convert-classic-menu-to-block-menu.js
Show resolved
Hide resolved
packages/block-library/src/navigation/edit/use-convert-classic-menu-to-block-menu.js
Show resolved
Hide resolved
packages/block-library/src/navigation/edit/use-convert-classic-menu-to-block-menu.js
Show resolved
Hide resolved
draganescu
approved these changes
Oct 6, 2022
|
@draganescu Just confirming that this won't now make it into 6.1 right? |
|
I believe that is the case |
|
I think it would benefit from waiting in the plugin more. What navigation building experience gained for 6.1 is definitely good, this would work best once the other work on slug based menu migration is also complete. |
45 tasks
82 tasks
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?
If a site has a classic navigation menu, but no wp_navigation CPTs then we should fallback to the classic navigation block. If there's a primary location then we should use the navigation assigned to it, if not then we could use the latest one.
Why?
For users who have set up classic menus this will be better than just showing them a list of pages.
How?
In the frontend we just fetch the correct navigation menu and convert it to blocks
In the editor we actually convert the navigation menu to a wp_navigation CPT. This creates some issues - at the moment it's created as a draft. Also it gets created multiple times. Perhaps the best way round this is to create it as a published menu, so that the fallbacks will continue to work as they do right now.
Testing Instructions