Save Navigation Block data to a wp_navigation post type #35746
Conversation
talldan
added
[Type] Enhancement
talldan
changed the title
|
Size Change: +1.67 kB (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
talldan
requested review from
jasmussen,
mtias,
tellthemachines,
karmatosed,
spacedmonkey,
adamziel,
draganescu,
getdave and
gwwar
|
Thanks for taking a stab at this. I'm tripping myself on some of the testing steps, and find myself unable to select a previously created menu. What am I doing wrong in this GIF? |
|
@jasmussen That's interesting. You should be seeing a two-step block placeholder, first step should look like this, similar to when a template part is added: |
|
@jasmussen That might happen if your code isn't re-built for some reason, maybe the build failed? It looks like you have the PHP changes but not the JavaScript changes. |
|
Thanks, I'll give this another shot. Back in a moment. |
|
This is an impressive PR:
Whether data should be saved as a custom post type or as a template part, I'm unqualified to chime in on. Focusing purely on the navigation block part of it, the primary effort would be to integrate a menu picker into the existing setup state so we end up with a one-step placeholder. I'll note down a task to do some mockups here. |
|
Took this for a spin and just like @jasmussen I am amazed how impressive it is. I did not have to figure it out at all, it just worked. The one problem I found is that the front end does not render the menu I see in the Site Editor. Is this known or am I doing it wrong? I see this in the editor:
And this rendered:
|
|
This one looks pretty interesting @talldan! Nice work. ✨ Things that need fixing❌ From testing I believe top level attributes on the nav block like vertical variation aren't being saved. Would it be simpler to insert a block reference, and have the navigation CPT store both the nav block and the inner-content? Context on FlowSo for folks reviewing, this PR plays around with the serialization of the block. Instead of storing link content inline, we instead create a nav CPT, and simply link to the post id:
In terms of workflow, there's a new step when adding a nav block, we give it a name. I understand that we're still prototyping here. Some suggested feedback for the new flow:
CleanShot.2021-10-19.at.10.39.39.mp4Questions from summary
There's a lot of similarities here for saving navigation content with a CPT and reusable blocks. Provided we are pretty clear on what is backed by a CPT and what's saved inline to a post, I think it might be a simple as showing some actions like "convert to reusable menu" and "convert to regular blocks" (or whatever we decide to call these sorts of nav blocks).
Probably a good opportunity to edit these CPTs with a block based editor like the nav editor. The post editor is serviceable enough in the meantime, but folks might need some explanation of what it is. We could maybe add a filter for this CPT, and add a wrapper nav block that doesn't get serialized? Or perhaps save the entire nav + contents in one place.
There's already "Menus" so how about "Block Menus"? No strong feelings on this one. Misc thoughtsOne thing that comes to mind is that if we use a CPT, there's very little difference here between backing a nav block with a CPT vs one with an existing menu. The latter would likely need a simple adapter, and some extra logic limiting the type of inner blocks we can add. Overall, I think the idea here is pretty promising. It be great to get some feedback from folks more familiar with how folks prefer building sites. Perhaps a +1 review from folks more familiar with themes, and another from APIs? |
There was a problem hiding this comment.
This is looking really good!
In addition to the todoes other folks have mentioned (block not rendering on the front end and not saving customizations such as color and variation jumped out the most to me), there's also:
- direct insert logic not working as expected (it always assumes direct insert is true, independently of what type of blocks are in the nav);
- in editor accessible from "All Navigation Menus", all blocks can still being inserted, as if it were a regular post. (I like the idea of leveraging the navigation editor for this view, but it doesn't feel like a must-have for V1 of this work, so perhaps we can just hide it for now.)
As for the CPT vs Template Part, there doesn't seem to be too much duplication of logic in this PR? I reckon it's worth going with the CPT.
➕ to making the 2-step placeholder flow 1 step only 😄
lib/navigation.php
Outdated
| function gutenberg_register_navigation_post_type() { | ||
| // TODO - Some of the language here needs to be revised. 'Navigation' is a | ||
| // singular noun, so cannot comfortably be used in a plural form, which | ||
| // leads to a situation where something like 'menus' needs to be suffixed. |
There was a problem hiding this comment.
Could we call the post type "menu" instead?
There was a problem hiding this comment.
In general can we decide on "navigation" vs "menu". I had understood we were normalizing towards "navigation" so as to disambiguate from classic Menus.
There was a problem hiding this comment.
@getdave Navigation is a singular noun, it doesn't have a plural form, so I think we're going to struggle to use it everywhere. Have a look through the strings below this comment to see what I mean.
There was a problem hiding this comment.
I went with Navigation Menu for now, it can be refined.
lib/navigation.php
Outdated
| 'insert_into_item' => __( 'Insert into Navigation', 'gutenberg' ), | ||
| 'uploaded_to_this_item' => __( 'Uploaded to this Navigation', 'gutenberg' ), | ||
| // Some of these are a bit weird, what are they for? | ||
| 'filter_items_list' => __( 'Filter Navigation list', 'gutenberg' ), |
There was a problem hiding this comment.
This one seems to provide the screen reader text for the "Filter" section in the Navigation post type index page:
The docs for register_post_type might need updating 😅
There was a problem hiding this comment.
Thanks for finding that!
| } } | ||
| /> | ||
| <Warning> | ||
| { __( 'Block cannot be rendered inside itself.' ) } |
There was a problem hiding this comment.
I was still able to add the block inside itself if I go through "All Navigation Menus" to the standalone editor. It rendered the whole block, with the warning in the place where the nested block would be. Then it started flickering, but I was able to save it.
Interestingly enough, when I tried the same with a menu that wasn't being used in any location, it didn't show that menu as an option to pick from in the site editor. But if the menu is already being used somewhere, it still shows.
There was a problem hiding this comment.
I assume that's because the isolated editor doesn't have a wrapper Nav block. There's work to do there in general about allowing insertion of child Nav blocks outside of the Nav parent block.
There was a problem hiding this comment.
Yeah, the standalone editor is just a post editor.
Interestingly enough, when I tried the same with a menu that wasn't being used in any location, it didn't show that menu as an option to pick from in the site editor. But if the menu is already being used somewhere, it still shows
@tellthemachines would you be able to elaborate on this?
There was a problem hiding this comment.
What I did was:
- Create a new nav post from the "All Navigation Menus" page.
- Give it a name and populate it only with blocks that can exist inside a Nav, such as Search and Social links.
- Save the post. I think I refreshed the page after that.
- Add a Navigation block to the post, click "Choose existing" and choose the one I am currently editing.
- The Navigation was added to the post, and inside it were all the blocks I had added above, plus the warning that the block can't render inside itself.
- I then went to to the Site Editor, and added a new Nav block to the index template.
- Under "Choose existing", the navigation that I had just created did not appear.
- I then went back and edited the post to remove the nested Nav block.
- Going back to the Site Editor and refreshing the page, that post now appeared under "Choose existing" in my new nav block.
Then I tried:
- From the "All Navigation Menus" page, opening an existing nav post that was already in use in a template.
- Adding a new nav block inside it, and choosing itself from "Choose existing".
- The block was added, but immediately started flickering, and clicking "update" made the whole editor hang, though it did save the post after a while. (The flickering and hanging didn't happen in the previous test.)
- I went to the Site Editor, and added a new Nav block to the index template.
- Under "Choose existing", I was able to choose the navigation that I had just mangled by rendering it inside itself.
- Both that, and the other existing instance of the same navigation in that template, displayed the warning about the block rendering inside itself, in addition to the other blocks that nav contained.
There was a problem hiding this comment.
Yeah, the editor at 'All Navigation Menus' is just the post editor, and as @getdave mentioned, because there's no wrapping Navigation Block, there's no recursion provider in that view, so it's possible for a user to create anything there.
I think for this PR, that option should be hidden in WP Admin.
The flickering sounds unusual. Not sure why that would happen.
You also mentioned this:
Under "Choose existing", the navigation that I had just created did not appear.
At the moment, drafts don't appear in that menu, so it might be that you saved it as a draft? I couldn't reproduce what you mentioned otherwise.
There was a problem hiding this comment.
Great work exploring these PRs Dan. Very much appreciated 🙇 👏 👏 👏
I've undertaken a review of this on video (with audio!).
What are we looking to achieve?
Before I make comments let's reflect on the wider goals we're looking to address (some of which may not be addressed in this PR):
- Allow navigations to be used in different locations within the same template/site.
- Allow reuse of the same navigation data but with different presentational treatments.
- Retain ability to quickly build new navigations.
- Separate presentation and data in order to afford editing navigation data in isolation (e.g. Nav Editor project).
- Allow reuse of navigations across Themes.
Overall, I think this PR is addressing (with some refinement) the first 4 items which is great (the point about Theme switching is being discussed in #35750).
Notes from Video
- The workflow was a lot more intuitive - less baggage from Template Parts is a win.
- I tried using the same menu in the header and footer but applying different presentational treatments (text colors...etc). This worked initially but when I reloaded some of these visual distinctions were lost.
- I really like the isolated view which has just the nav items and not the wrapper. This for me is where the Nav Editor should be headed. Avoids all baggage from the Nav block and provides a very useable experience to manipulate the data.
- as in Try using a template part in the navigation block #35418 when viewing the CPT in isolated view you cannot add Nav block items because they are not part of parent. We should be able to work around that somehow.
Technical approach
I would also like to emphasise that I believe you are taking the correct route in not serializing the Nav block itself to the CPT. The block should be a wrapper concerned solely with presentation specific to the location (within the template) in which it is being used, where as the inner blocks should represent the data structure of the navigation itself. If we were to serialize the Nav block then we'd also persist all the presentation and it would therefore make it far less flexible when attempting to reuse the same navigation data between instances (eg: header vs footer) or indeed within an Isolated Editor view (e.g. Nav Editor).
lib/navigation.php
Outdated
| function gutenberg_register_navigation_post_type() { | ||
| // TODO - Some of the language here needs to be revised. 'Navigation' is a | ||
| // singular noun, so cannot comfortably be used in a plural form, which | ||
| // leads to a situation where something like 'menus' needs to be suffixed. |
There was a problem hiding this comment.
In general can we decide on "navigation" vs "menu". I had understood we were normalizing towards "navigation" so as to disambiguate from classic Menus.
| // We don't want to render a missing state if we have any inner blocks. | ||
| // A new template part is automatically created if we have any inner blocks but no entity. |
There was a problem hiding this comment.
| // We don't want to render a missing state if we have any inner blocks. | |
| // A new template part is automatically created if we have any inner blocks but no entity. | |
| // If we have a reference to the Navigation Post but the post entity no longer exists | |
| // then show a warning to the user. |
Question: if the users ends up in this state can we provide them with an option to create a new navigation rather than having to remove the block and re-add it?
packages/block-library/src/navigation/edit/placeholder/index.js
Outdated
Show resolved
Hide resolved
packages/block-library/src/navigation/edit/placeholder/index.js
Outdated
Show resolved
Hide resolved
| ? __( 'Choose an existing menu or create a new one.' ) | ||
| : __( 'Create a new menu.' ) |
There was a problem hiding this comment.
| ? __( 'Choose an existing menu or create a new one.' ) | |
| : __( 'Create a new menu.' ) | |
| ? __( 'Choose an existing navigation or create a new one.' ) | |
| : __( 'Create a new navigation.' ) |
packages/block-library/src/navigation/edit/placeholder/select-navigation-post-step.js
Outdated
Show resolved
Hide resolved
|
Question: If we store menus in this new post type, will titles of posts, pages etc will be dynamitically be updated like the current menus. Will pages / posts that are deleted, automatically be removed from this content? Will links to CPT that are no longer be register also be removed. Menus have be somewhat dynamic, as they link to other WordPress content. If menu is not dynamic, then this is a massive step backwards for users, IMO. |
talldan
force-pushed
the
update/nav-block-to-use-custom-post-type
branch
from
69e04ab to
63c685b
Compare
@tellthemachines This is going to be a difficult one to improve. I think it needs more discussion. I've made an issue outlining the challenges and detailing a potential solution - #35942 |
noisysocks
added
the
Needs Dev Note
|
Nice work! Let's get this in early and iterate since other issues and PRs depend on it. I added "needs dev note" since it would be good to explain the CPT intricacies on Make/Core. |
Description
Closes #34612. Alternative to #35418, that instead of using a template part post type, uses a dedicated
wp_navigationcustom post type for saving the navigation block's inner blocks.TODO:
How has this been tested?
New blocks
Upgrading blocks
trunk, build the codebase.3 Checkout this branch
Screenshots
Types of changes
New feature (non-breaking change which adds functionality)
Checklist:
*.native.jsfiles for terms that need renaming or removal).