Fix navigation editor undo button being active when editor loads #34839
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: +182 B (0%) Total Size: 1.06 MB
ℹ️ View Unchanged
|
|
This does fix the issue, but it looks like the entity config also causes a request to an It might be time to formalise the idea of an in-memory entity. What do you think @adamziel? |
|
So I think the alternative to this is that we replace It might be nice, but it would be a lot of work and potentially a point where errors might be introduced. I'm not sure if there's any prior art for this kind of thing. |
talldan
requested review from
ajitbohra,
draganescu,
getdave,
kevin940726,
nerrad,
ntwb and
tellthemachines
as code owners
I agree. We already have two editors that use "in-memory" or stub entities. I'm just getting more familiar with the Maybe we can have a more general "in-memory" entity? const entity = {
label: __( 'In Memory' ),
name: '__experimentalInMemory',
kind: 'root',
transientEdits: { blocks: true, selection: true },
}Can we use Should we move the core data changes into separate PR and add some unit tests. Thanks for working on this, @talldan, and sorry if my comment isn't very helpful. |
Yep, that is an option. I do prefer an explicit property, because it means we're not relying on an undocumented behavior. For example, the entity system seems like it could have a default
Yep, happy to do that. I think I'll gather a bit more feedback on this one first. |
That's a good point, thanks for the clarification. |
talldan
force-pushed
the
fix/navigation-editor-transient-edits
branch
from
9236efc
to
01fa1fc
Compare
|
So there was a bug with this where making the entity have the |
talldan
changed the title
getdave
approved these changes
Sep 21, 2021
There was a problem hiding this comment.
I tested this and
✅ There are no undo/redo states on reload.
✅ There are no erroneous network requests.
It would seem reasonable to me to add this experimental entity prop. It is after all "experimental" and so if we decide to formalise the concept of an in-memory entity later, we can remove it.
| name: NAVIGATION_POST_POST_TYPE, | ||
| transientEdits: { blocks: true, selection: true }, | ||
| label: __( 'Navigation Post' ), | ||
| __experimentalNoFetch: true, |
There was a problem hiding this comment.
Let's add a comment here to explain (briefly) why we do this.
Short term as an experimental thing, for sure, let's go for it. Longer-term, I'm leaning more towards removing in memory post entirely, and editing the menu and menu item entities directly. I had a chat with @noisysocks about that, and we're not sure how much effort would be involved. It would couple the block with the entities directly, which means we wouldn't have to do much transforming and diffing on save. I'm not sure if that's good or bad yet, but sounds like an attractive simplification. Let's explore, document what we learn, and regroup then. |
|
The tricky part is the top level Definitely agree that it'd be good to explore We should probably make an issue to discuss this in the context of both widgets and menus so that both editors don't end up with completely different implementations. |
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
Fixes #34802
The navigation editor is an entity driven editor, like the post or widget editor. But the entity that drives it is never saved, it's only a convenient in-memory entity. It also didn't have any configuration (edit: it actually presently uses the
postTypesentity, which is a bit of a hack), which caused the undo feature in the nav editor to work differently to other editors (more details in point 2 here - #34802 (comment)).In this PR I've made it so that the entity config is created at initialisation of the editor. As it's only a convenience entity, I don't think it makes sense to add it to the
entities.jsfile. This seems like a decent alternative.It also changes the kind and type so that the navigation post is more like a
postTypeentity.To avoid this entity making REST API calls I've added an
__experimentalNoFetchproperty to it.How has this been tested?
Expected: The
undobutton should be disabled.Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist:
*.native.jsfiles for terms that need renaming or removal).