-
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
Buttons: allow split and merge #22436
Conversation
Size Change: +142 B (0%) Total Size: 1.11 MB
ℹ️ View Unchanged
|
I notice you're not using |
@ZebulanStanphill Tbh, I'm not sure why gutenberg/packages/block-editor/src/components/rich-text/index.js Lines 251 to 257 in 532844f
|
In the future, I'm hoping that we can make it easier for blocks to implement split and merge because this is a lot of different settings and attributes to pass. We'll have a better idea how it could look once more blocks implement split/merge. |
9346021
to
8883940
Compare
I struggle to interpret it now as well 😬 I trace it back to #8735, if it helps provide context for what it was needed for at the time. There were also some end-to-end tests included, so I'd assume if those can still pass without the code, it should be okay to remove. |
@@ -183,6 +191,19 @@ function ButtonEdit( props ) { | |||
: undefined, | |||
...colorProps.style, | |||
} } | |||
onSplit={ ( value ) => { | |||
if ( ! value ) { | |||
return createBlock( 'core/button' ); |
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.
Are there any values that would be better to retain from the forked button's attributes? The style, colour and border settings come to mind. But then we'd have to be careful not to port the remaining attributes, since it could cause unexpected and sometimes incorrect behaviour (e.g. if a hook adds HTML Anchor support to a button for some reason).
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.
Right, I don't really know what to do here. It sounds like generally we should keep the attributes for both blocks as it makes more sense for buttons (not for paragraphs). The anchor will be an issue with this code as well since it's possible to split in the middle and end up with two buttons with the same attributes (both parts are non empty). The anchor issue should probably be fixed at another level, maybe something that makes sure the content has no duplicate anchors in general?
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 with keeping the attributes for both buttons.
Co-authored-by: Miguel Fonseca <miguelcsf@gmail.com>
Description
Allows buttons to be split with enter and backspace. Currently these insert line breaks which doesn't make much sense in the context of buttons.
How has this been tested?
Screenshots
Types of changes
Checklist: