Block Editor: Make initial inner blocks non-dirtying. #19521
Conversation
epiqueras
requested review from
ellatrix,
talldan and
youknowriad
as code owners
| @@ -350,15 +350,18 @@ const withBlockCache = ( reducer ) => ( state = {}, action ) => { | |||
| */ | |||
| function withPersistentBlockChange( reducer ) { | |||
| let lastAction; | |||
| let markNextChangeAsNotPersistent = false; | |||
There was a problem hiding this comment.
Should we name this with the "is" prefix? Something like isNextActionExplicitlyNotPersistent?
There was a problem hiding this comment.
That is harder to read.
|
|
||
| return ( state, action ) => { | ||
| let nextState = reducer( state, action ); | ||
|
|
||
| const isExplicitPersistentChange = action.type === 'MARK_LAST_CHANGE_AS_PERSISTENT'; | ||
| const isExplicitPersistentChange = action.type === 'MARK_LAST_CHANGE_AS_PERSISTENT' || markNextChangeAsNotPersistent; |
There was a problem hiding this comment.
This flag seems a little bit confusing because it is true if the change is explicitly persistent or explicitly not persistent, right? The name seems to indicate the flag is true only when the change is explicit persistent, but not when the change is explicitly not persistent.
I guess we could keep the flag as it was before and do them or when we need something explicitly persistent or explicitly not persistent? Or name the flag something like isExplicitPersistentOrNotChange ? (not a very good name but is more detailed).
There was a problem hiding this comment.
The name seems to indicate the flag is true only when the change is explicit persistent, but not when the change is explicitly not persistent.
It's an "explicit persistent change". The change could be to true or false.
| * @return {Object} Action object. | ||
| */ | ||
| export function __unstableMarkNextChangeAsNotPersistent() { | ||
| return { type: 'MARK_NEXT_CHANGE_AS_NOT_PERSISTENT' }; |
There was a problem hiding this comment.
To mark a change as persistent, we use MARK_LAST_CHANGE_AS_PERSISTENT to mark the previous change as persistent, and we trow the action after the action we want to mark. While to explicitly mark a change as not persistent, we use MARK_NEXT_CHANGE_AS_NOT_PERSISTENT, and we trow it before the action we want to mark.
Would it be possible to make the usage of the actions more similar and trow both actions before or after the actions we want to mark?
There was a problem hiding this comment.
No, because of usage nuance:
In our framework, when you want a persistent change to not be persistent, it's so that the non-dirtying change handler is called from the block editor. If you fire an edit, and then fire a MARK_NEXT_CHANGE_AS_NOT_PERSISTENT, nothing guarantees that the wrong change handler wasn't already called before the MARK_NEXT_CHANGE_AS_NOT_PERSISTENT.
MARK_LAST_CHANGE_AS_PERSISTENT is different, because when using it, it's ok that the non-dirtying handler might have already been called, it will just call the dirtying one again with the same values.
There was a problem hiding this comment.
Hi @epiqueras, thank you for clarifying the question I raised 👍
While testing this PR, I think I found a regression.
In templates that have a template part block, I can never make the template dirty e.g: while the template I change things in paragraphs inside and outside the template part block and the update button continues disabled.
|
It looks like I missed a code path in which we should also clear the flag. It should be fixed now! |
There was a problem hiding this comment.
Hi @epiqueras, thank you for the iteration the regression seems solved, and the current behavior seems correct. Nice work 👍
I noticed two dirtiness issues that I think we should address in this PR or a follow-up PR:
Bug 1: Undo in the template makes the template part dirty.
- Create a template with some paragraphs and a template part with some paragraphs nested.
- Save everything.
- Reload and verify the post is not dirty.
- Change something in a paragraph inside the template (outside the template part).
- Verify only the template becomes dirty as expected.
- Press undo, verify the template part also becomes dirty (it should not).
Bug 2: Changing things inside the template part also makes the template dirty.
- Create a template with some paragraphs and a template part with some paragraphs nested.
- Save everything.
- Reload and verify the post is not dirty.
- Change something in a paragraph inside the template-part (outside the template part).
- Verify the template part and the template become dirty, only the template part should be dirty.
|
Thanks for the thoroughness! Let's work on that in follow-ups as they are managed by different code paths. |
Follows #19203
Description
This PR makes the new experimental initial blocks value bootstrap added to
InnerBlocksby #19203 a non-dirtying operation. Without this, any posts that have a block that uses it would be dirty upon opening.How has this been tested?
Posts with template parts are no longer dirty before making any changes.
Types of Changes
New Feature: The
InnerBlockscontrolled mode no longer makes the post dirty before making any changes.Checklist: