[Navigation editor] Save menu items using the REST API #34541
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
|
Size Change: -1.19 kB (0%) Total Size: 1.06 MB
ℹ️ View Unchanged
|
Every thunk receives const thunk = () => ( { registry } ) => {
const error = registry.select( 'core' ).getLastEntitySaveError( 'root', 'menu', menuId );
/* ... */
} |
Uuu that's nice and I totally missed that, thank you! |
talldan
reviewed
Sep 7, 2021
adamziel
force-pushed
the
update/migrate-navigation-screen-to-rest-api
branch
from
ae9dba0 to
852db8c
Compare
adamziel
commented
Sep 7, 2021
|
Still pretty draft-y, but I like how thunks + locks = -500 lines so far |
|
Not quite there yet, but we're getting close. |
adamziel
force-pushed
the
update/migrate-navigation-screen-to-rest-api
branch
from
16f4642 to
ed9ea01
Compare
adamziel
added
[Block] Navigation
adamziel
requested review from
TimothyBJacobs,
spacedmonkey,
kevin940726,
hellofromtonya,
talldan and
anton-vlasenko
adamziel
requested review from
getdave,
nerrad and
tellthemachines
as code owners
adamziel
commented
Sep 9, 2021
|
Aside of the API problems, I think this is in a pretty good shape. Let's discuss! CCing previously involved folks @noisysocks @talldan @draganescu @TimothyBJacobs @spacedmonkey |
adamziel
commented
Sep 9, 2021
| } ); | ||
| } ); | ||
|
|
||
| describe( 'saveNavigationPost', () => { |
There was a problem hiding this comment.
I removed these saveNavigationPost tests for now as they are customizer-specific. Let's discuss the ideas in this PR and then double down with more relevant tests.
There was a problem hiding this comment.
We could use unit tests for the async REST API-based version, but I think it's okay to follow-up with that to avoid inflating this PR more than it already it.
adamziel
force-pushed
the
update/migrate-navigation-screen-to-rest-api
branch
from
ceddec8 to
4ff8b16
Compare
|
I love that this is in. I wanted to mention that the saving of blocks as menu items is something that started as an experiment, but that in the meantime the understanding of what exactly the block navigation opt in will do has evolved quite significantly. So, the focus of optimization and fixing after this |
This was referenced Sep 22, 2021
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
This PR migrates navigation saving from
admin-ajax.phpto REST API v2. It ended up being pretty large PR, we may or may not need to split it into a few smaller ones.Known issues
Partial failure is very much possible. A lot of validation happens in
prepare_item_for_databaseso we can't rely onrequire-all-validatefeature of batch requests. The more validation we move tovalidate_callback, the more reliable this will be.I don't think there is a good way of communicating the partial failure to the user so we should minimize the risk as much as possible. Here's how it looks like for now:
Other considerations
Editor is in a dirty state after loading the page. This is not specific to this PR – trunk behaves the same.
What about using
<EntityProvider />? Would it make the code simpler?We could get rid of the "stub post", that's for sure. About batch saving – we'd still need to compute the diff between what's persisted and what's in memory. I think that
<EntityProvider />would just callblockToMenuItemearlier than we do now, but we'd still need to compute a list of batch tasks and check for errors.Other changes:
serializeProcessingwas replaced by locks fromcore-dataRelated to #24907
Test plan
Go to the experimental navigation screen, play with it, and keep saving the data. Expect to see frequent API errors, but no JS errors.