Navigation: Always create a fallback menu #47684
Merged
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
|
Flaky tests detected in 3a5252b. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4192731842
|
|
Size Change: +30 B (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
draganescu
reviewed
Feb 3, 2023
draganescu
reviewed
Feb 3, 2023
draganescu
reviewed
Feb 3, 2023
draganescu
reviewed
Feb 3, 2023
|
A problem with this is that I cannot ever be in a state where I have no block menus. In the editor if I delete the last block navigation it will just create a new one with a page list in it. |
Yes, when you try to use it. Is that a problem? |
scruffian
force-pushed
the
add/create-fallback-navigation
branch
from
c536475
to
3b5a0a9
Compare
|
Actually I am wrong @scruffian because if I have block navigation then the block has a ref and when I delete the menu that the ref points to I get the warning. If there are no menus that I can switch to, I see the warning and I can:
Cool! 👍🏻 |
draganescu
previously requested changes
Feb 3, 2023
Good catch. Fixed in 4fd95cb |
|
@scruffian this PR should also remove the current page list fallback behavior . 🙏🏻 |
|
I think this is ready for another review. |
scruffian
force-pushed
the
add/create-fallback-navigation
branch
from
3a15b83
to
3d041f2
Compare
draganescu
reviewed
Feb 9, 2023
|
|
I added a commit to fix the tests. |
kevin940726
reviewed
Feb 10, 2023
scruffian
force-pushed
the
add/create-fallback-navigation
branch
from
716f4ec
to
e072b47
Compare
This is not what happens in my testing. |
scruffian
force-pushed
the
add/create-fallback-navigation
branch
2 times, most recently
from
01988b7
to
083adec
Compare
|
I managed to replicate the issue once, but not again. I think it's probably connected to the order in which the classic menus API request loads. I have updated the dependencies for the useEffect that handles the creation of classic menus, to ensure that they are all loaded before it happens. Hopefully this fixes the issue for you? |
draganescu
approved these changes
Feb 16, 2023
There was a problem hiding this comment.
Ok, I couldn't break this w/ testing and the code looks good to me. Let's ship it and see how it works in real life. Overall I think this is a good change compared to the cooler but way buggier path of having a "fallback" that is not an actual menu but some limbo state confusing users and developers :)
I think this is only possible today after all the refinements added to the navigation block in relation to defaults, theme switching behavior and UI so awesome job 👏🏻
| @@ -237,6 +209,40 @@ function Navigation( { | |||
| [ navigationMenus ] | |||
| ); | |||
|
|
|||
| // This useEffect adds snackbar and speak status notices when menus are created. | |||
| // If there are no fallback navigation menus then we don't show these messages, | |||
| // because this means that we are creating the first, fallback navigation menu. | |||
There was a problem hiding this comment.
Can we / Should we - also hide the snackbar about the importing of the default classic menu? If I have a classic menu and it is used as the basis for the default block navigation I get a snackbar saying classic menu imported ..
There was a problem hiding this comment.
I thought about that, but I'm not sure. It's a different case and I thought it might be useful to know what's happened in that case.
|
I wonder if we should, later, unify the code that handles defaulting and fallback in some REST endpoint in the menus group of endpoints? It would really make this a lot less frail and also stop us from introducing inadvertently conflicting behavior between the front end and the editor. @spacedmonkey @TimothyBJacobs - is an endpoint that checks and solves the situation of "a navigation block wants to render something but there is nothing there so let's find the most appropriate thing to render" in conflict with any REST principle or can we have such an utility endpoint? |
scruffian
force-pushed
the
add/create-fallback-navigation
branch
from
1d59818
to
3a5252b
Compare
|
@draganescu It's not very RESTful, I'd probably just make a one-off endpoint in |
scruffian
force-pushed
the
add/create-fallback-navigation
branch
from
3a5252b
to
f5fe083
Compare
|
@TimothyBJacobs a one off endpoint in |
|
@draganescu Right so a REST endpoint that you can query and say get me a fallback menu and it will return one. Can be used in editor and front of site. This would
|
|
This PR creates multiple wp_navigation CPTs when there are multiple blocks in the same post. Reverting in #48602 until we have a way to deal with that problem. |
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 creates a fallback navigation CPT when there isn't one. Fixes #47058
Why?
This should simplify several things
How?
We automatically create a wp_navigation CPT in both the editor and the frontend, if a user doesn't have one.
Testing Instructions
Testing Instructions for Keyboard
As above.