Fix button line break causes unexpected behaviour#4320
Conversation
blocks/library/button/index.js
Outdated
| placeholder={ __( 'Add text…' ) } | ||
| value={ text } | ||
| focus={ focus } | ||
| onEnter={ |
There was a problem hiding this comment.
note: it would be great if this could be a behaviour people can opt in easily when building blocks.
onEnter={ insertBlocksAfter( 'core/paragraph' ) }There was a problem hiding this comment.
Why can't we just reuse onSplit for this, that's how it works for the paragraph block. Worried about adding another prop.
There was a problem hiding this comment.
We can use onSplit, but it is hacky and does a series of computations not required for this case. For example, if I press enter with the cursor in the middle of the text, editable will separate the text for us, and call our on split handler with after and before. Then we would append after and before again and set the attributes with the append result while calling insertBlocksAfter to insert a new block. On paragraph and headings on split works great because in fact, we want to split the content which is not the case for the button.
My idea to address the concern from @mtias to make this functionally simple to use would be to keep changes in editable and pass a new prop to blocks called insertBlockIfPossible. This would allow block creator to opt in for this behavior with one line onEnter={ this.props.insertBlockIfPossible }.
There was a problem hiding this comment.
so we'd have onMerge and onReplace dealing with the "backspace" key and onSplit and onEnter dealing with the "enter" key. I think these APIs are confusing.
I think we should bring some consistency here. I wonder if we can't make it more "declarative" something like:
writingFlow: {
defaultBlockOnEnter: true,
splitOnEnter: true,
backspaceRemoveIfEmpty: true,
mergeWithPrevious: true,
}
I'm just thinking out loud, maybe worth exploring separately.
There was a problem hiding this comment.
The problem with a declarative approach is that the way we merge or split is not generic it is dependent on the block e.g: attribute names. Also, a block can have more than one editable so the operations are editable specific e.g for the same block splitting operations on different editables may be different.
Even the enter handling is dependent on the focus e.g while in button block if we are editing the text and we press enter we may expect a new block to appear if we are on the URL field and press enter we don't expect a new block.
There was a problem hiding this comment.
We could do direct bool props on editable for these behaviours like:
<Editable defaultBlockOnEnter />There was a problem hiding this comment.
Hi @mtias,
We could do direct bool props on editable for these behaviours like:
This is an option if we also pass a prop to editable that allows new blocks to be inserted. Righ now, editable has no way to create new blocks.
Another option would be to just disable enter e.g do nothing on button when enter is pressed. This way we can just pass a bool without any additional prop and use your approach <Editable defaultBlockOnEnter />
A third idea is to follow @youknowriad idea and pass a set of flags like:
writingFlow: {
defaultBlockOnEnter: true,
splitOnEnter: true,
backspaceRemoveIfEmpty: true,
mergeWithPrevious: true,
}
But also pass writingFlowFunctions props that contain the functions that allow removal of block () = onReplace( [] ), insertation of blocks etc...
Blocks would receive a defaultWritingFlowFunctions prop that they would be able to pass directly to editable that would contain functions for trivial default cases, like remove, enter etc...
c1b0155 to
c439fae
Compare
|
After talking with @iseulde I got the idea that we should avoid blocking the enter behavior. So I updated this PR change the CSS instead and better handle multiple lines (although it is not perfect), the look now shows the other lines: |
c439fae to
100b900
Compare
Thank you for the suggestion, it was added. |
|
🎉👍👍 |
|
Any reason why the line height is so big? It almost looks like I'm making paragraphs within the button. Cc @jasmussen |
|
Shift enter works no? I didn't test it. |
|
Oh indeed — I think perhaps it has a tall lineheight because it was designed as a single-line button which it clearly isn't. Yes, it would be nice to refactor so the button uses padding instead of lineheight to vertically center the text in a single line button. |
100b900 to
712c81a
Compare
|
Thank you all for your feedback, the CSS was updated. I'm not expecting changes in the single line case, for multiple lines the button now looks like this: cc: @karmatosed |






Description
This PR fixes "Button line break causes unexpected behavior" - #4295.
.
In order to solve the problem, the case was updated to allow multiple lines:
How Has This Been Tested?
Add a button block, write some text, press enter and verify the block expanded to shown that line.