Nav block - find menu by slug using existing core data API #45443
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
|
|
|
Size Change: +257 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
getdave
commented
Nov 1, 2022
getdave
commented
Nov 1, 2022
| }; | ||
|
|
||
| const recursionId = `navigationMenu/${ ref }`; | ||
| const hasAlreadyRendered = useHasRecursion( recursionId ); | ||
| const { editEntityRecord } = useDispatch( coreStore ); | ||
|
|
||
| const generatedSlug = useGeneratedSlug( clientId ); |
There was a problem hiding this comment.
We can simplify this by removing the generation of the slug and just allowing the REST API to provide the slug to us that comes from WP Core.
getdave
commented
Nov 1, 2022
getdave
commented
Nov 1, 2022
packages/block-library/src/navigation/edit/navigation-menu-delete-control.js
Outdated
Show resolved
Hide resolved
getdave
commented
Nov 1, 2022
packages/block-library/src/navigation/edit/navigation-menu-name-control.js
Outdated
Show resolved
Hide resolved
getdave
commented
Nov 1, 2022
| const label = decodeEntities( title.rendered ); | ||
| if ( id === currentMenuId && ! isCreatingMenu ) { | ||
| if ( | ||
| ( slug === currentMenuSlug || id === currentMenuId ) && |
There was a problem hiding this comment.
A comment explaining that this is necessary due to possibility of the reference to the Nav Menu Post being either a slug or an ID (legacy).
getdave
commented
Nov 1, 2022
Comment on lines
19
to
20
| DEFAULT_ENTITY_KIND, | ||
| DEFAULT_ENTITY_TYPE, |
There was a problem hiding this comment.
Revert to hard coded values.
getdave
commented
Nov 1, 2022
packages/block-library/src/navigation/edit/unsaved-inner-blocks.js
Outdated
Show resolved
Hide resolved
getdave
commented
Nov 1, 2022
| @@ -57,8 +57,12 @@ export default function useCreateNavigationMenu( clientId ) { | |||
| ); | |||
| } ); | |||
| } | |||
|
|
|||
| const maybeSlug = slug || title; | |||
There was a problem hiding this comment.
Let's explain why this is here. In the future we may want to pass a slug to represent the template part hierarchy.
getdave
commented
Nov 1, 2022
Comment on lines
17
to
18
| DEFAULT_ENTITY_KIND, | ||
| DEFAULT_ENTITY_TYPE, |
There was a problem hiding this comment.
Revert to hardcoded values.
getdave
commented
Nov 1, 2022
packages/block-library/src/navigation/edit/use-generate-default-navigation-title.js
Outdated
Show resolved
Hide resolved
getdave
commented
Nov 1, 2022
| import { useSelect } from '@wordpress/data'; | ||
| import { store as blockEditorStore } from '@wordpress/block-editor'; | ||
|
|
||
| function useGeneratedSlug( clientId ) { |
There was a problem hiding this comment.
@draganescu Do you agree that we should remove this logic from this PR and just keep it simple and allow the REST API (via WP Core) to determine the slug for us?
getdave
commented
Nov 1, 2022
packages/block-library/src/navigation/use-navigation-entity-types.js
Outdated
Show resolved
Hide resolved
getdave
commented
Nov 1, 2022
getdave
commented
Nov 1, 2022
getdave
commented
Nov 1, 2022
getdave
requested review from
scruffian,
MaggieCabrera,
draganescu and
ajlende
|
I think this approach makes a lot of sense and seems simpler. When I test it, I don't see any slug attribute being set on the block, so it seems to work entirely based on fallbacks. If you create a new menu and then set your menu to use the old one then the newest one still displays. I am testing in the Site Editor. |
|
There's a chance I broke this last thing with some tweaks I made. I'll re-test today. |
getdave
force-pushed
the
try/nav-block-slugs-existing-api
branch
from
844e0e8
to
ecd728f
Compare
|
One thing I am noticing is that once a blog has a ref attribute, no matter what I change, the ref attribute is never removed. |
We spoke off Github and agree the |
Ah that was expected. I decided that if a block had a ref once then it should remain indefinitely even if it's been soft deprecated (i.e. no longer used once a |
|
Note: there may be a bug here whereby we're using the same entity query for both "all menus" and "single menu by slug". |
UpdateThis PR is still in progress but I've temporarily switched focus to work on the |
|
Going to be picking this one up again shortly. |
getdave
force-pushed
the
try/nav-block-slugs-existing-api
branch
from
689fc1b
to
6d5a796
Compare
|
Flaky tests detected in 68db735. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5056822304
|
|
Consider the following pattern export const getCurrentTheme =
() =>
async ( { dispatch, resolveSelect } ) => {
const activeThemes = await resolveSelect.getEntityRecords(
'root',
'theme',
{ status: 'active' }
);
dispatch.receiveCurrentTheme( activeThemes[ 0 ] );
}; |
getdave
force-pushed
the
try/nav-block-slugs-existing-api
branch
4 times, most recently
from
c8154bf
to
2bd396b
Compare
getdave
force-pushed
the
try/nav-block-slugs-existing-api
branch
from
2bd396b
to
683c3be
Compare
getdave
force-pushed
the
try/nav-block-slugs-existing-api
branch
from
68db735
to
b8e945e
Compare
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?
Alternative to #42809.
Makes the Navigation block use a
slugto reference a Navigation Menu Post. This approach avoids modifications to the REST API endpoint and instead usesgetEntityRecords(plural) to fetch the menu byslug.Why?
See #42809.
How?
slugattribute to the block.getEntityRecordsto look up a Navigation Post byslug(post_name). Uses the 0th result. This is a one time operation to find the entity record.IDof the returned Navigation Post into local state (not saved to the block) for use in entity operations (e.g. edit, update, create).<EntityProvider>,getEditedEntityRecord,saveEntityRecord) continue to utiliseIDas the primary record key.This approach is different to that in
trunkwhich usesgetEntityRecordto look up the Navigation Post by ID. As the block no longer stores a reference to an ID (it's now aslug) this approach would not work.Problems
The main issue with this PR is that permissions requests cannot be dispatched until a Post ID is available. As we now use slugs to look up a post from the API there is no PostID available until after the request to fetch a given menu has resolved (or not!).
Therefore we have to wait for the request to resolve and then check the permissions. This seems backwards and is currently causing UI errors such as a permission warning when converting Page List into a menu.
The only way to have this work would be to modify the REST API to handle slugs for permissions requests such as in https://github.com/WordPress/gutenberg/pull/44975/files#r995650785. However, this is not something the REST API team want to pursue.
Todo
NavigationMenuSelector..Testing Instructions
Test new block
This is testing the flow of
creating a new Nav block that utilises slugs
updating an existing Nav block that uses slugs
New Post
Add Nav block
See Page List fallback
Convert to links (click on Page List and click "Edit").
See Navigation menu created.
Switch to Code View and check a
slugattribute is on the block.Try adding menu items
Try saving and reloading.
Try creating another Navigation Menu - is it unique attribute-wise.
Now delete all your Navigation Menus at http://localhost:8888/wp-admin/edit.php?post_type=wp_navigation&paged=1.
Reload your Post. You should see the error that your menu is not longer available.
Test legacy block
This tests older versions of the block that use the
refattribute.trunk. Rebuild all the things.refattribute.refattribute is not removed or modified.slug. Ref is only preserved until such time as you select a new entity for your menu. After that the block will switch to using slugs.Screenshots or screencast