Avoid automatic saving of uncontrolled Nav blocks #39883
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: +474 B (0%) Total Size: 1.22 MB
ℹ️ View Unchanged
|
getdave
requested review from
ntwb,
nerrad,
ajitbohra and
tellthemachines
as code owners
|
@talldan I'm particularly pinging you here because I know you did a lot of work on handling uncontrolled inner blocks and you're very likely to be able to notice edge cases 🙏 |
|
...I need to fix the e2e tests and add a new one to assert that making inner blocks dirty will trigger conversion to entity. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
getdave
commented
Apr 1, 2022
Comment on lines
+60
to
+70
| // If the current inner blocks object is different in any way | ||
| // from the original inner blocks from the post content then the | ||
| // user has made changes to the inner blocks. At this point the inner |
There was a problem hiding this comment.
I should update this comment. It's not about whether object contents are different. It's about the object references changing if the block data changes.
This comment was marked as outdated.
This comment was marked as outdated.
scruffian
reviewed
Apr 4, 2022
|
|
||
| // Initially store the uncontrolled inner blocks for | ||
| // dirty state comparison. | ||
| if ( ! originalBlocks?.current ) { |
There was a problem hiding this comment.
Is modifying the ref directly like this advised? Would it be better to put the ref directly on the component?
There was a problem hiding this comment.
What makes you concerned here?
The ref is a stored value which we create solely to store whatever blocks the component is passed as its initial value.
We do nothing with originalBlocks in terms of passing it to <InnerBlocks>. Its sole purpose is to act as a source of truth for the original "state" of the blocks that was originally passed to the component.
I'm going to double check this checks out correctly and I'm not setting us up for bugs.
There was a problem hiding this comment.
I guess my concern is whether we are abusing the ref feature of React - why not just us a local variable?
There was a problem hiding this comment.
Because a ref is guaranteed to be the same reference for the lifetime of the component whereas the local variable will be a new reference on each render of the component:
The returned object will persist for the full lifetime of the component (React Docs)
There was a problem hiding this comment.
So basically I'm using the ref to store the "source of truth" for the lifecycle of the component. Then if at any point the current blocks don't match up with the original source of truth reference stored in the ref then the blocks have changed because the references are different.
There was a problem hiding this comment.
Would it be possible to replace this ref with a combination of hasUncontrolledInnerBlocks and hasUnsavedBlocks that's used in edit/index.js?
There was a problem hiding this comment.
Basically I wonder if this information can be derived from somewhere instead of computing it here
There was a problem hiding this comment.
hasUnsavedBlocks is computed here as a product of
- whether there is an entity
- whether there are uncontrolled inner blocks present (this is determined by
getBlock( clientId ).innerBlocks)
Neither of those factors tell us whether the inner blocks have had modifications. They simply report that
- there is no
wp_navigationentity associated with the current block. - there are inner blocks present.
But can't we use the "saved" state to determine whether there are changes to the blocks? Unfortunately not. Why? Because that's putting the cart before the horse.
We need the uncontrolled version of the Nav block to save (convert to entity mode) only when we detect there have been modifications to those blocks. Therefore we can't use the "saved" state to determine whether the blocks have changed because that state should be a product of whether the blocks have changed.
As far as I can see the only way to determine if the uncontrolled inner blocks have changed is to effectively memoize the initial value of blocks (i.e. getBlock( clientId ).innerBlocks) and then check that to see if it changes.
This is what I'm trying to do with the ref. I save the initial value and then compare that reference against the new incoming value. When the user makes a change to the inner blocks the object returned by getBlock( clientId ).innerBlocks will change and thus we can compare the references and determine they are no longer equal.
I might be able to do that outside the component but I'm not sure what the value of that would be.
There was a problem hiding this comment.
Wow, great exploration @getdave 👍 alright, let's stick to using the ref then. If this happens more often, then we could look into abstracting it out into a reusable API.
scruffian
reviewed
Apr 4, 2022
scruffian
reviewed
Apr 4, 2022
packages/block-library/src/navigation/edit/unsaved-inner-blocks.js
Outdated
Show resolved
Hide resolved
scruffian
reviewed
Apr 4, 2022
React docs explain this can lead to undesired side effects
getdave
force-pushed
the
try/avoid-automatic-saving-of-uncontrolled-nav-blocks
branch
from
0fd77ee
to
82a0d8d
Compare
getdave
force-pushed
the
try/avoid-automatic-saving-of-uncontrolled-nav-blocks
branch
from
82a0d8d
to
b44cbe1
Compare
talldan
reviewed
Apr 5, 2022
packages/block-library/src/navigation/edit/unsaved-inner-blocks.js
Outdated
Show resolved
Hide resolved
You cannot reset a block that has uncontrolled inner blocks
draganescu
reviewed
Apr 5, 2022
| // If the block has inner blocks, but no menu id, then these blocks are either: | ||
| // - inserted via a pattern. | ||
| // - inserted directly via Code View (or otherwise). | ||
| // - from an older version of navigation block added before the block used a wp_navigation entity. |
There was a problem hiding this comment.
Can't they be added directly by the theme as well?
getdave
deleted the
try/avoid-automatic-saving-of-uncontrolled-nav-blocks
branch
|
This makes adding a new item quite weird: flicker-link-control.mp4 |
This was referenced Apr 5, 2022
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?
This PR updates the Nav block so that if it has uncontrolled blocks (i.e. those not coming from a
wp_navigationentity but rather those provided directly as<-- wp:block) then it will not automatically convert them into an entity.Instead the block will now wait for the blocks to become "dirty" (i.e. have been changed) before converting into a
wp_navigation.Part of #38157 and/or #39489
Why?
There are a number of ways where a Nav block might contain raw inner blocks. For example:
Taking the former example from TT2 here is the block definition:
Notice how the Page List block is provided in order that there is default content in the block when the Site Editor is loaded.
The current problem with this is that as soon as the user interacts with the Nav block it will automatically convert the uncontrolled blocks into a
wp_navigationpost. There is no warning that this has occured and so the user is none the wiser.The result of this is that it's super easy to inadvertently create many
wp_navigationposts simply by selecting Navigation blocks on your site.How?
This PR fixes thing by allowing the raw inner blocks to stay uncontrolled until such time as the user makes a modification to those blocks. As soon as that occurs then they will be auto-converted into a
wp_navigationpost and a notice will be displayed announcing that to the user.In future it might be nice to give the user some means to opt in/out of the conversion perhaps via a dialog. This PR follows te simplest path to fix the core problem of accidental creation of
wp_navigationposts.Testing Instructions
refattribute on the Nav block.Editand thenConvert.wp_navigationentity will be created.refattribute on the Nav block.Screenshots or screencast