Update unsaved navigation block flow #35976
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
talldan
added
[Type] Bug
|
Size Change: +107 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
tellthemachines
approved these changes
Oct 27, 2021
There was a problem hiding this comment.
This is working well! Tested both with patterns in Site Editor and with existing Navigation blocks in Post Editor.
The Post Editor is showing a PHP message:
Notice: Trying to get property 'post_status' of non-object in /var/www/html/wp-content/plugins/gutenberg/build/block-library/blocks/navigation-submenu.php on line 134
Warning: Cannot modify header information - headers already sent by (output started at /var/www/html/wp-content/plugins/gutenberg/build/block-library/blocks/navigation-submenu.php:134) in /var/www/html/wp-admin/admin-header.php on line 9
but that's also happening on trunk, so not caused by these changes.
| > | ||
| { __( | ||
| 'The navigation block has been updated to store data in a similar way to a reusable block. Please use the upgrade option to save your navigation block data and continue editing your block.' | ||
| { __( 'Save this block to continue editing.' ) } |
There was a problem hiding this comment.
This message is great for purposes of nav placeholders in patterns, but can be a bit confusing for existing navigation blocks (why do I suddenly need to save this block again?). I suppose we're not able to distinguish between the two? Not a blocker for this PR, but something to iterate on maybe.
There was a problem hiding this comment.
Yeah, I'm not sure what the best option is here. Let's continue improving it. It might be that this message isn't even a thing in the next iterations.
|
Took it for a quick spin, and seeing this: It's good to see patterns working as they are, and compared to trunk this is a good step forward. UX wise it needs further refinement. The big message being output when selecting a navigation block could be better, but mostly it shifts the layout which is a general problem that we've been looking at fixing, and is also problematic for the current two step placeholder. In general I'm unsure whether we need absolutely have to save every navigation ever made, and I'd like to explore what it could look like if we were a bit more lenient allowing you to save a menu as reusable after the fact. But for the purposes of enhancing what's already merged, and seeking out the boundaries of that approach, this seems like a natural next step. Let's keep the initial issue open though, as we keep looking for solutions 👍 👍 |
|
Thanks for the feedback, lets merge this and then continue discussing further improvements. |
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.
Description
Improves the problem described in #35947.
#35746 included a prompt for the user to 'Upgrade' a navigation block that uses 'uncontrolled' inner blocks (inner blocks that are not saved to the wp_navigation post type). Unfortunately this prompt was visible in pattern previews.
This PR streamlines the prompt and makes it only show on block selection so that it doesn't appear in the preview of patterns.
I don't intend this to be a perfect fix, but it should at least be an immediate improvement to the way patterns that include the navigation block appear.
How has this been tested?
Screenshots
Before
After
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist:
*.native.jsfiles for terms that need renaming or removal).