Auto-inserting blocks: Remove toggle if block is present elsewhere #54024
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
ockham
added
[Feature] Block API
This was referenced Aug 29, 2023
|
Size Change: +21 B (0%) Total Size: 1.51 MB
ℹ️ View Unchanged
|
gziolo
reviewed
Aug 30, 2023
|
Flaky tests detected in ff3d13378c9a0c66bdbc842b44bd70f73e3cf8c9. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6024828150
|
ockham
force-pushed
the
update/auto-inserting-blocks-remove-toggle-if-block-is-present-elsewhere
branch
from
5d3eac0 to
10b0da3
Compare
|
Rebased (since I was seeing a few weird JS unit test errors). Should be ready for review now! |
|
@ockham, I resolved the remaining warning with How
I've also memoized |
|
The code is quite complex, and that's perfectly fine as I don't think we can much simplify it without creating even more abstractions in the block editor's store. I want to focus on testing next, and thinking about how we could cover that with e2e tests as a follow-up so we can continue iterating on the existing logic and depend less on manual testing which takes a ton of time. |
gziolo
approved these changes
Sep 1, 2023
gziolo
reviewed
Sep 4, 2023
| ...clientIds, | ||
| [ block.name ]: autoInsertedBlock.clientId, | ||
| }; | ||
| } else if ( getGlobalBlockCount( block.name ) > 0 ) { |
There was a problem hiding this comment.
I had this after-thought that we could put:
if ( getGlobalBlockCount( block.name ) === 0 ) {
return clientIds;
}at the top of the function. If there is no block present in the editor, then we are sure we can show the toggle. In this place, we could simplify the check in the highlighter line to:
} else {With proper inline comments that might also simplify following all the edge cases.
Anyway, it's a stylystic issue and I'm fine with the current approach, too. It's just something that helped me to understand the logic.
There was a problem hiding this comment.
Makes sense! Applied in 35ebe4e8bc 😊
ockham
force-pushed
the
update/auto-inserting-blocks-remove-toggle-if-block-is-present-elsewhere
branch
from
31a5424 to
5f72af4
Compare
|
The diff after all the recent changes is much easier to follow, I like how it shaped after a few additional tweaks. |
ockham
deleted the
update/auto-inserting-blocks-remove-toggle-if-block-is-present-elsewhere
branch
ockham
added a commit
that referenced
this pull request
Sep 4, 2023
…54024) If a block that would normally be auto-inserted is not found in its designated position but elsewhere, we can assume somewhat safely that it was inserted manually, and that the user is aware of the block and won't need any UI elements to make it more visible. Additionally, this PR fixes most of the following warnings (raised [here](#52969 (comment))): ``` The 'useSelect' hook returns different values when called with the same state and parameters. This can lead to unnecessary rerenders. ``` --------- Co-authored-by: George Mamadashvili <[email protected]>
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?
Follow-up to #52969.
Remove the Auto-inserting block inspector panel toggle for a block if it's present in a different location than expected for auto-insertion.
Additionally, this PR fixes most of the following warnings (raised here):
Why?
If a block that would normally be auto-inserted is not found in its designated position but elsewhere, we can assume somewhat safely that it was inserted manually, and that the user is aware of the block and won't need any UI elements to make it more visible.
This was first discussed here.
How?
By checking if the global block tree contains any instance of the block type in question. (Hat-tip @gziolo for pointing me to
getGlobalBlockCount🙌 )Testing Instructions
.wp-env.override.jsonfile.Screenshots or screencast