-
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
Image Block: Fix cover block exists check #32666
Conversation
Size Change: +40 B (0%) Total Size: 1.04 MB
ℹ️ View Unchanged
|
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.
LGTM 👍
* | ||
* @return {boolean} Whether the block exists. | ||
*/ | ||
function checkBlockExists( name, list ) { |
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.
If this is formalized in a function, it seems like it belongs in the block/api/registration set and not on the block
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.
Hi, @mtias
Do you want me to create a new issue for this function?
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 think it'd make sense in order to keep the image block file cleaner. Alternatively, we might want to approach this slightly different and look at getPossibleBlockTransformations
instead and not to the block registry.
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 relying on the transforms seems to be a really good idea, it may cover other checks that we may be missing. I guess we can rely on the getBlockTransformItems selector.
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.
Just to be clear, the getPossibleBlockTransformations
selector already exists :)
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 agree. Using a transforms selector makes more sense here. Also, we don't have all cases covered yet - #32520.
I will create a new PR that uses getPossibleBlockTransformations
if possible or similar logic.
Description
Updates
coverBlockExists
check to include checking againstallowedBlockTypes
list.Fixes #32094.
How has this been tested?
Types of changes
Bugfix
Checklist:
*.native.js
files for terms that need renaming or removal).