-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove synced blocks from lastBlockInserted #52558
Conversation
Size Change: +604 B (0%) Total Size: 1.43 MB
ℹ️ View Unchanged
|
Flaky tests detected in 109384e. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5543295053
|
@@ -3328,7 +3328,7 @@ describe( 'state', () => { | |||
expect( state.clientIds ).toEqual( [ clientIdOne, clientIdTwo ] ); | |||
} ); | |||
|
|||
it( 'should return client ids of all blocks when inner blocks are replaced with REPLACE_INNER_BLOCKS', () => { | |||
it( 'should not return client ids of all blocks when inner blocks are replaced with REPLACE_INNER_BLOCKS', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we in any way make this a positive assertion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Put some initial blocks in state. Freeze them. Run the reducer function. Check the blocks in state are the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the test. Does this look better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes thank you so much 🙏🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is unclear to me if the name of the reducer should be lastManuallyInserted
:D in the sense that we want to to give us as close as possible the ID of the last block the user inserted. What the application does in the background should not be counted but also the name is not making this obvious. Maybe the function doc should specify?
However I think this PR is good since it solves the problem it introduced a while ago. No tests or functionality seem to be broken because of this here not is it a foundation aspect of data flow - it's too new.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm running into an issue with the search block button only option in the editor. The search block is unselected after a little less than a second, causing the input to collapse:
Screen.Recording.2023-07-12.at.1.18.09.PM.mov
@scruffian This is likely the |
Done here: #52604 |
I gave this another test by rebasing #52604 against this branch / PR, and it fixed the above issue. Nice work! |
What?
Fixes #51713.
How?
Removes REPLACE_INNER_BLOCKS from the condition under which we determine that a block has been inserted into the canvas.
useBlockSync
callsreplaceInnerBlocks
, when it's trying to sync the controlled blocks state:gutenberg/packages/block-editor/src/components/provider/use-block-sync.js
Line 127 in de71547
Since #46885, this ends up setting the state for the last inserted block in the reducer:
gutenberg/packages/block-editor/src/store/reducer.js
Line 1858 in de71547
This means that when a controlled block is synced, the app considers all of these blocks to be "last inserted blocks". However the user would not consider these blocks to have been "last inserted".
This causes issues like #51713, because we keep on thinking that the search block has been inserted again and resetting the attributes.
Testing Instructions