Skip to content

Conversation

@aduth
Copy link
Member

@aduth aduth commented Jun 28, 2018

This pull request seeks to remove the NewBlock event handling from the RichText component. The function neither has DocBlock nor is publicly documented in the RichText documentation, so I was unable to determine its reason for being. Prompting justification on its introduction in #409 (comment) was further unsuccessful in determining that it needs to exist. We currently have no core components which are both multiline="p" and have an onSplit function passed to its RichText, so it appears to be unreachable. Per its introduction in #409, it may have been useful for when the text block could allow enter presses and a second press responsible for creating the split block.

Based on explorations in #7583, we may want to leverage TinyMCE's NewBlock event, but I believe we should do so consistent for all splitting behavior. In the meantime, it should be removed.

Testing instructions:

Verify there are no regressions for block splitting, namely:

  • Non-multiline blocks (paragraph)
    • Pressing enter once should split to a new paragraph
  • Multiline blocks (list)
    • Pressing enter once should add new list item
    • Pressing enter twice should escape out into a new paragraph block

There should be end-to-end tests for this. See also #6467.

@aduth aduth added the [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable label Jun 28, 2018
@aduth aduth requested review from ellatrix and youknowriad June 28, 2018 19:15
@aduth aduth force-pushed the remove/rich-text-new-block branch from 4662692 to 713ffc5 Compare June 28, 2018 19:16
@aduth aduth requested a review from a team July 9, 2018 19:56
@ellatrix
Copy link
Member

ellatrix commented Jul 9, 2018

Nice. I thought this was used by the quote block but it doesn't appears so now... I think the block could be split on double enter to escape the quote, much like how you describe the list block. Doesn't work in master atm, and there's no e2e test.

@gziolo gziolo added this to the 3.3 milestone Jul 10, 2018
@aduth aduth merged commit d574792 into master Jul 10, 2018
@aduth aduth deleted the remove/rich-text-new-block branch July 10, 2018 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants