[Try] menuSlug instead of navigationMenuId #36522
Closed
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
adamziel
commented
Nov 16, 2021
Comment on lines
11
to
+13
| "navigationMenuId": { | ||
| "type": "number" | ||
| "type": "number" | ||
| }, |
There was a problem hiding this comment.
This needs to be removed of course
adamziel
commented
Nov 16, 2021
Comment on lines
24
to
26
| const navigationMenu = slug | ||
| ? getEditedEntityRecord( ...navigationMenuSingleArgs ) | ||
| : null; |
There was a problem hiding this comment.
Need to create a new entity if slug is set but there is no navigationMenu with that slug
adamziel
commented
Nov 16, 2021
| if ( ! oldSlug ) { | ||
| const newSlug = area?.area | ||
| ? `${ area.area }//menu` | ||
| : `generic-one-off//menu`; |
There was a problem hiding this comment.
This should be unique. GUID + server side hooks perhaps?
getdave
reviewed
Nov 16, 2021
| $template_query = new WP_Query( $wp_query_args ); | ||
| $post_id = $template_query->posts[0]->ID; | ||
|
|
||
| $request['slug'] = $request['id']; |
There was a problem hiding this comment.
Suggested change
| $request['slug'] = $request['id']; | |
| $request['slug'] = $request['slug']; |
There was a problem hiding this comment.
This code doesn't seem to get executed because @adamziel returns the value here: https://github.com/WordPress/gutenberg/pull/36522/files#diff-678d8bd4bed4738f7b7c74ec8f9eb0bfdcc45d87ae275466cce7cb3601e654ecR125
I suppose that is intentional, @adamziel ?
There was a problem hiding this comment.
It is sort of intentional, that's a very quick&dirty draft and I just wanted to get some data flowing, I guess I just didn't remove dead code.
adamziel
commented
Nov 16, 2021
| // TODO: this util should perhaps be refactored somewhere like core-data. | ||
| import { createTemplatePartId } from '../template-part/edit/utils/create-template-part-id'; | ||
|
|
||
| export default function useTemplatePartArea( clientId ) { |
There was a problem hiding this comment.
This is mostly copied over from useTemplatePartAreaLabel, there's a lot of deduplication that could be done here.
adamziel
commented
Nov 16, 2021
| /** | ||
| * Base Templates REST API Controller. | ||
| */ | ||
| class Gutenberg_REST_Navigation_Controller extends WP_REST_Posts_Controller { |
There was a problem hiding this comment.
This is quick&dirty attempt to reproduce template areas mechanics of requesting items by their slug, e.g. /wp/v2/navigation/header-menu. A lot of cleaning up needed here.
anton-vlasenko
requested changes
Nov 16, 2021
| if ( ! current_user_can( 'edit_theme_options' ) ) { | ||
| return new WP_Error( | ||
| 'rest_cannot_manage_templates', | ||
| __( 'Sorry, you are not allowed to access the templates on this site.', 'gutenberg' ), |
There was a problem hiding this comment.
It says templates.
Should we change this to
Sorry, you are not allowed to access the navigation menus on this site. ?
There was a problem hiding this comment.
Fully agreed, it's just a copy&paste from the other endpoint for now
| // This capability is required to edit/view/delete templates. | ||
| if ( ! current_user_can( 'edit_theme_options' ) ) { | ||
| return new WP_Error( | ||
| 'rest_cannot_manage_templates', |
| protected function permissions_check() { | ||
| // Verify if the current user has edit_theme_options capability. | ||
| // This capability is required to edit/view/delete templates. | ||
| if ( ! current_user_can( 'edit_theme_options' ) ) { |
There was a problem hiding this comment.
I don't know if we have specific capability to edit navigation menus, but if we do, I think we should use it instead of edit_theme_options
| $template_query = new WP_Query( $wp_query_args ); | ||
| $post_id = $template_query->posts[0]->ID; | ||
|
|
||
| $request['slug'] = $request['id']; |
There was a problem hiding this comment.
This code doesn't seem to get executed because @adamziel returns the value here: https://github.com/WordPress/gutenberg/pull/36522/files#diff-678d8bd4bed4738f7b7c74ec8f9eb0bfdcc45d87ae275466cce7cb3601e654ecR125
I suppose that is intentional, @adamziel ?
|
The biggest problem here is with automatically creating a new menu when it's missing. After a quick chat with @youknowriad it doesn't seem like core data supports initializing a new in-memory entity record and saving it later. A reasonable workaround for now would be calling |
adamziel
commented
Nov 16, 2021
| name: slug, | ||
| post_name: slug, | ||
| }; | ||
| dispatch( coreStore ) |
There was a problem hiding this comment.
This needs to be less hacky
youknowriad
reviewed
Nov 16, 2021
| "type": "number" | ||
| "type": "number" | ||
| }, | ||
| "menuSlug": { |
There was a problem hiding this comment.
Why not just "slug" since we're on the navigation block already.
There was a problem hiding this comment.
also just "slug" is what template parts do
|
The endpoint sometimes returns a numeric ID instead of slug. Once that's fixed, perhaps the editor bits would just work. |
Yeah, this is how the nav block works already when inserting from a pattern, so it was already a trade-off. We're working with a foundation that doesn't yet support the concepts we need and it's too late to introduce them for 5.9, so trade-offs are the only option IMO, they just need to be smart trade-offs. I had a good think about in-memory entities. A problem is that the user can save the content without saving the in-memory entity (e.g. a post containing a nav block with in-memory data autosaves). If the user wants to recover that revision, it won't contain the in-memory data. Probably what we need is a more seamless switch between the nav block in this form, where it has normal inner blocks as part of the block's content: <!-- wp:navigation -->
<!-- wp:navigation-link /-->
<!-- /wp:navigation -->and this form where it saves to a CPT: <!-- wp:navigation { "id": 1 } /-->Maybe that's in conjunction with an in-memory entity. The in-memory experience serializes from/to the first form of the block, the persisted experience serializes from/to the second. |
adamziel
force-pushed
the
try/navigation-slug
branch
from
94415c6
to
6e8aecd
Compare
|
Size Change: +883 B (0%) Total Size: 1.1 MB
ℹ️ View Unchanged
|
|
This PR is almost 1 years old and I moved on to other tasks since then so I'm closing it. If you could use the changes proposed here, feel free to pick up this PR! cc @draganescu @getdave @scruffian @MaggieCabrera – I wonder if that would be relevant for your work on theme switching. |
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.
See #36524
CC @getdave @noisysocks @anton-vlasenko @mtias @gziolo @draganescu