BlockInvalidWarning: Prefer canInsertBlockType and refactor to hooks.
#49052
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
…sic block not insertable
talldan
force-pushed
the
fix/convert-to-classic-block-in-site-editor
branch
from
9e1a3f7 to
56b9e12
Compare
|
Size Change: -12 B (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
Mamaduka
reviewed
Mar 14, 2023
Comment on lines
66
to
73
| const convertToClassic = useCallback( () => { | ||
| replaceBlock( block.clientId, blockToClassic( block ) ); | ||
| }, [ block, replaceBlock ] ); | ||
|
|
||
| const convertToHTML = useCallback( | ||
| () => replaceBlock( block.clientId, blockToHTML( block ) ), | ||
| [ block, replaceBlock ] | ||
| ); |
There was a problem hiding this comment.
Suggestion (non-blocking): We can inline these inside useMemo and use a single method for memoization; also eliminate the blockToClassic and blockToHTML helpers.
There was a problem hiding this comment.
Yeah, I thought about that, tested it out in 9099bc1
Mamaduka
reviewed
Mar 14, 2023
Comment on lines
62
to
64
| const attemptBlockRecovery = useCallback( () => { | ||
| replaceBlock( block.clientId, recoverBlock( block ) ); | ||
| }, [ block, replaceBlock ] ); |
There was a problem hiding this comment.
Nit: There's no need for the useCallback for this function; we can inline it. The button component isn't memoized and will re-render alongside the parent.
There was a problem hiding this comment.
Yeah, I realise that. I did it for consistency, as otherwise the code looked ugly 😄
talldan
commented
Mar 14, 2023
|
|
||
| const convertToBlocks = useCallback( | ||
| () => replaceBlock( block.clientId, blockToBlocks( block ) ), | ||
| const convert = useMemo( |
There was a problem hiding this comment.
I could destructure it, but I decided to keep it this way as it allows me to pass a single convert value into the following useMemo dependency array..
|
Can this take in account the issue #49005? |
Thanks for mentioning that one, I don't think I'll tackle that in this PR, as this is now finalized, but I'll have a think about how that one can be solved. |
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 #49051.
This component should really use
canInsertBlockTypefor checking whether a block can be converted to HTML or Classic blocks.At the moment it checks whether the block type is registered, but this doesn't handle situations like
allowedBlocksfor inner blocks.I've also refactored this component to use hooks.
Why?
It should prevent any issues, like a user accidentally inserting an HTML or Classic blocks into a Buttons block.
How?
Use the
canInsertBlockTypeselector instead of thegetBlockTypeTesting Instructions
Test 1
Test 2
trunkafter the refactorScreenshots or screencast