Skip to content

Fix button line break causes unexpected behaviour#4320

Merged
karmatosed merged 1 commit intomasterfrom
fix/linebreaks-button
Feb 7, 2018
Merged

Fix button line break causes unexpected behaviour#4320
karmatosed merged 1 commit intomasterfrom
fix/linebreaks-button

Conversation

@jorgefilipecosta
Copy link
Copy Markdown
Member

@jorgefilipecosta jorgefilipecosta commented Jan 5, 2018

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:
image.

How Has This Been Tested?

Add a button block, write some text, press enter and verify the block expanded to shown that line.

@jorgefilipecosta jorgefilipecosta added [Feature] Blocks Overall functionality of blocks [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable [Type] Bug An existing feature does not function as intended labels Jan 5, 2018
@jorgefilipecosta jorgefilipecosta self-assigned this Jan 5, 2018
placeholder={ __( 'Add text…' ) }
value={ text }
focus={ focus }
onEnter={
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: it would be great if this could be a behaviour people can opt in easily when building blocks.

onEnter={ insertBlocksAfter( 'core/paragraph' ) }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can't we just reuse onSplit for this, that's how it works for the paragraph block. Worried about adding another prop.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 }.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could do direct bool props on editable for these behaviours like:

<Editable defaultBlockOnEnter />

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

@jorgefilipecosta
Copy link
Copy Markdown
Member Author

jorgefilipecosta commented Feb 2, 2018

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:
image.

@jasmussen
Copy link
Copy Markdown
Contributor

Nice, works well

While you're at it, can you add word-break: break-word; to the button styles`?

screen shot 2018-02-05 at 09 40 23

@jorgefilipecosta
Copy link
Copy Markdown
Member Author

While you're at it, can you add word-break: break-word; to the button styles`?

Thank you for the suggestion, it was added.

@jasmussen
Copy link
Copy Markdown
Contributor

🎉👍👍

@ellatrix
Copy link
Copy Markdown
Member

ellatrix commented Feb 5, 2018

Any reason why the line height is so big? It almost looks like I'm making paragraphs within the button. Cc @jasmussen

@jasmussen
Copy link
Copy Markdown
Contributor

Shift enter works no? I didn't test it.

@ellatrix
Copy link
Copy Markdown
Member

ellatrix commented Feb 5, 2018

Yes, that works like other inline editables.

What I mean is:

screen shot 2018-02-05 at 14 50 56

@ellatrix
Copy link
Copy Markdown
Member

ellatrix commented Feb 5, 2018

Expected something like:

screen shot 2018-02-05 at 14 53 12

screen shot 2018-02-05 at 14 52 37

@jasmussen
Copy link
Copy Markdown
Contributor

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.

@jorgefilipecosta
Copy link
Copy Markdown
Member Author

jorgefilipecosta commented Feb 7, 2018

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:
image

cc: @karmatosed

@karmatosed karmatosed merged commit d05abf2 into master Feb 7, 2018
@karmatosed karmatosed deleted the fix/linebreaks-button branch February 7, 2018 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Feature] Blocks Overall functionality of blocks [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable [Type] Bug An existing feature does not function as intended

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants