Editable: Enter splits the current paragraph into two blocks #409

youknowriad · 2017-04-12T10:58:20Z

This PR builds on #407
Tries to address #375 #374

The idea for this PR is to catch Enter key down event and split the text blocks into two text blocks.

How it works?

If we specify a tagName on the Editable component, I'm assuming we want TinyMCE to handle the Enter key press (see list block for example)
For the text/block I'm dropping the tagName to allow custom Enter behavior.
When we hit enter, we let TinyMCE update the dom ( see setTimeout ), and then we tries to figure out how to split the content ( computing before and after ) . These values are passed to a new callback prop onSplit
In this PR, I'm providing a new eventHandler to the edit property of blocks. I'm calling it insertBlockAfter. The block author can call this function to append a new block after the current block.
I was thinking of splitting the block only on two enters, but this assumes the text block can store multiple paragraphs which is not the case for now. Storing multiple paragraphs on the text block will lead us to reconsider how to serialize/parse the text-align attribute. Will we duplicate it on the different paragraphs? Should we add a div wrapper.
This is the first PR introducing Keyboard interaction, and we should consider how to update the focus and the caret position across blocks. Should we follow the same idea used in the multi-instance prototype where we store the focused block and the focus position in the state?

nylen · 2017-04-12T11:17:26Z

I think a text block should include multiple paragraphs.

aduth · 2017-04-12T12:36:05Z

I think a text block should include multiple paragraphs.

In this case, what should happen when you apply a center alignment to a block containing multiple paragraphs? What's the flow for centering a single paragraph in this sequence?

youknowriad · 2017-04-12T12:40:37Z

One possible answer:

what should happen when you apply a center alignment to a block containing multiple paragraphs

I think we should keep it block level, I mean this applies the centering to all the paragraphs, and we store this config as a comment attribute.

What's the flow for centering a single paragraph in this sequence?

You have to split them into multiple blocks

aduth · 2017-04-12T12:45:10Z

You have to split them into multiple blocks

From a technical perspective this seems reasonable, but I'm a little concerned if it's possible to create an obvious UX around this.

jasmussen · 2017-04-12T13:17:04Z

From a technical perspective this seems reasonable, but I'm a little concerned if it's possible to create an obvious UX around this.

Question: in a text block where multiple paragraphs are part of the same block, can we without too many headaches still have a UI that is on the HTML block level? I.e. you can still click an individual paragraph as though it was a block, to surface the options there?

Early mockup here: #365 (comment)

Edit: Another option is to do the same as TinyMCE does currently. If your cursor is on a paragraph that is centered, the centered button is pressed. When you put focus on a left aligned paragraph, the button is unpressed again.

youknowriad · 2017-04-12T13:24:58Z

Another option is to do the same as TinyMCE does currently. If your cursor is on a paragraph that is centered, the centered button is pressed. When you put focus on a left aligned paragraph, the button is unpressed again.

This seems doable to me. The alignment would be handled the same way we handle "bold", "italic"...
From a technical point of view, this means we don't have any attribute stored on the block level, we just infer the state of the controls from the current caret position.

aduth · 2017-04-12T13:36:25Z

From a technical point of view, this means we don't have any attribute stored on the block level, we just infer the state of the controls from the current caret position.

This seems to break down the flow of render/control logic consuming and operating exclusively on the attributes of a block, since it allows an escape where an isActive handler can now introspect into the block's own rendered content. I am wary of this solution.

youknowriad · 2017-04-12T13:40:44Z

This seems to break down the flow of render/control logic consuming and operating exclusively on the attributes of a block, since it allows an escape where an isActive handler can now introspect into the block's own rendered content. I am wary of this solution.

How do you think the inline controls ( bold, italic... ) should work? I don't think these are considered controls ( I mean controls defined on the controls attribute of the block ). What I'm saying here is that maybe the text alignments should be considered exactly like these inline controls.

Let me know if you have another idea about the inline controls and their state.

joyously · 2017-04-12T14:07:12Z

I think a text block should include multiple paragraphs.

Doesn't this go with the idea that a user could want to select multiple blocks and wrap a div around them for styling? I haven't heard any discussion about block level controls to "group" and "ungroup" blocks. It is difficult in this editor because it's all about what you can see, and a grouping block isn't seen. But it should be able to be styled. The "clearing" block fits into this category of unseen but styled, also.

ellatrix · 2017-04-12T14:19:06Z

To me it feels like the disconnect between this approach and contenteditable really become clear with issues like this and UX issues like selecting across blocks. Possible solutions seem to blur the line more and more, so I don't think it would be bad to just have onecontenteditable region and integrate it well.

aduth · 2017-04-12T14:55:40Z

blocks/components/editable/index.js

@@ -57,6 +59,28 @@ export default class Editable extends wp.element.Component {
 		this.props.onChange( value );
 	}

+	onKeyDown( event ) {
+		if ( ! this.props.tagName && event.keyCode === 13 ) {


Could we do an early return to avoid indenting the function?

if ( this.props.tagName || event.keyCode !== 13 ) { return; }

aduth · 2017-04-12T14:56:55Z

blocks/components/editable/index.js

@@ -91,3 +115,7 @@ export default class Editable extends wp.element.Component {
 		);
 	}
 }
+
+Editable.defaultProps = {


Aside: I'm wary of the future of defaultProps, especially since in most cases it can be achieved by destructured object defaults. We're doing this already for the component with the tagName prop (see render) so we should at least choose a consistent approach.

I'm ok with default values while destructuring

aduth · 2017-04-12T14:58:12Z

blocks/components/editable/index.js

+			// Wait for the event to propagate
+			setTimeout( () => {
+				// Getting the content before and after the cursor
+				this.editor.selection.getStart();


Problem with a setTimeout is we can't be guaranteed that the component didn't unmount in the last tick, meaning due to the behavior of the function ref, this.editor could potentially be null.

React will call the ref callback with the DOM element when the component mounts, and call it with null when it unmounts.

https://facebook.github.io/react/docs/refs-and-the-dom.html

Also... what is this line achieving if It's not being assigned? Are there side effects to calling getStart?

The first point of my comment still applies for calling on this.editor in the context of setTimeout:

Problem with a setTimeout is we can't be guaranteed that the component didn't unmount in the last tick, meaning due to the behavior of the function ref, this.editor could potentially be null.

#409 (comment)

aduth · 2017-04-12T15:03:42Z

blocks/components/editable/index.js

+				const after = getHtml( childNodes.slice( splitIndex ) );
+
+				// Splitting into two blocks
+				this.editor.setContent( this.props.value );


I'm confused by this line. Wouldn't this reset the content back to its original value prior to the split? I don't see that in practice, but confused as to why it wouldn't.

Yes, that's the intent. By this, I revert the content to the previous state before hitting Enter, because this change is "controlled", the split callback should update the content accordingly.

If I drop this line, it will work for the moment, but I know that without it we can have issues (Forgot exactly why, this is comming from the multi-instance). I think maybe focus issues.

aduth · 2017-04-12T15:05:43Z

editor/test/state.js

+			} );
+
+			expect( Object.keys( state.byUid ) ).to.have.lengthOf( 3 );
+			expect( state.order ).to.eql( [ 'kumquat', 'kumquat3', 'kumquat2' ] );


Embrace the theme! I demand more originality! 😆

hahahaha :)

nylen · 2017-04-12T15:53:38Z

In this case, what should happen when you apply a center alignment to a block containing multiple paragraphs? What's the flow for centering a single paragraph in this sequence?

Option 1: apply to all paragraphs; require splitting into a separate block to align multiple paragraphs separately.

Option 2: remove the alignment buttons. Only half-joking here.

From a technical perspective this seems reasonable, but I'm a little concerned if it's possible to create an obvious UX around this.

Here are two other ways to think about this issue.

Layout blocks

Longer-term we want to extend the concept of "content blocks" to "layout blocks" - providing an alternative to a widget seems like a good first step here. I'm going to use the term "widget" to mean the block editing experience that we will eventually want to bring to other pieces of a WP page layout.

Should widgets be able to contain multiple paragraphs of text? Yes, absolutely.

Should widgets contain multiple "blocks" as we are currently building them? I think probably not.

Most common editing operations

Writing multiple paragraphs of text in a row is extremely common. Therefore, this operation should be extremely easy.

The only way we should have one paragraph per block is if the friction of inserting a new block and navigating across blocks is near zero. We already know this isn't the case. To me this implies very clearly that a single text block should be able to contain multiple paragraphs.

nylen · 2017-04-12T15:55:50Z

Doesn't this go with the idea that a user could want to select multiple blocks and wrap a div around them for styling? I haven't heard any discussion about block level controls to "group" and "ungroup" blocks. It is difficult in this editor because it's all about what you can see, and a grouping block isn't seen. But it should be able to be styled. The "clearing" block fits into this category of unseen but styled, also.

@joyously - I think we are not ready to plan for this yet. I'm also operating under the assumption that a block will be "root level" for the foreseeable future - i.e. it will be a direct child of the article.post element that holds the post content.

joyously · 2017-04-12T19:41:00Z

I think we are not ready to plan for this yet.

That sounds backwards. Keep it in mind when designing the basics, because it will be wanted.

I'm also operating under the assumption that a block will be "root level" for the foreseeable future - i.e. it will be a direct child of the article.post element that holds the post content.

That's a strange assumption. Nested lists come to mind immediately. An image is a block, right? And can you put it in a paragraph?

youknowriad · 2017-04-13T09:39:25Z

In 4d9eaef a text block can have multiple paragraphs and I'm using a parent div to apply the alignments. I think this is a good compromise for now.

So now, if you hit "Enter" once, the block is not split. You need another "Enter" press to split.

youknowriad · 2017-04-13T09:41:42Z

blocks/library/text/index.js

 				value={ content }
 				onChange={ ( value ) => setAttributes( { content: value } ) }
 				style={ align ? { textAlign: align } : null }
+				onSplit={ ( before, after ) => {
+					setAttributes( { content: before } );
+					insertBlockAfter( {


I'll use the createBlock utility once #407 land

nylen · 2017-04-13T18:47:12Z

In 4d9eaef a text block can have multiple paragraphs ... if you hit "Enter" once, the block is not split. You need another "Enter" press to split.

🎉

I suppose the next thing we will want to handle is Backspace at the beginning of a block to collapse two blocks together. I'm still concerned about the use cases in #179 but allowing multiple paragraphs helps a lot.

and I'm using a parent div to apply the alignments. I think this is a good compromise for now.

I thought adding wrapper divs was something we wanted to avoid, it will probably break people's styles for example. Why not add the alignment attributes to each p tag in the block?

nylen · 2017-04-13T18:50:20Z

I think we are not ready to plan for this yet.

That sounds backwards. Keep it in mind when designing the basics, because it will be wanted.

I'm also operating under the assumption that a block will be "root level" for the foreseeable future - i.e. it will be a direct child of the article.post element that holds the post content.

That's a strange assumption. Nested lists come to mind immediately. An image is a block, right? And can you put it in a paragraph?

@joyously - this is valuable feedback, but it is pretty far out of scope for this particular pull request. Can you please create two separate issues for these tasks?

Wrapper elements around blocks
Nested blocks

and a couple of the use cases you're imagining for each one.

youknowriad · 2017-04-13T18:59:12Z

Why not add the alignment attributes to each p tag in the block?

It's a possibility but harder to implement, I just opted for the simplest for now. I can take a deeper look but the problem here is the Editable component treats the content as a unique string, so we need to extract the ps in the save method from the content in order to do so. I wanted to avoid the parsing. (And when we say parsing an HTML string, there's always the question on how to do it. Do we use DOM APIs, do we use anything else).

nylen · 2017-04-13T19:17:03Z

Yep, this is another set of issues that we've been discussing in #391 and #413. I'm surprised that it ended up being related here, but given that, your approach here seems fine.

I'm also a bit concerned about how we now require an extremely specific structure (<div><p>) for text blocks, otherwise the content inside the block is not successfully loaded. However I think that can probably also be addressed separately.

Can we get some test cases for parsing HTML content into the structure for this block? Like blocks/api/test/parser.js

I'm concerned about the p { margin-bottom: 0; margin-top: 0 } styles defined here - this causes writing a single long section of text to not work as expected. (I expected to press Enter and get a new paragraph with a blank line before it; instead, in order to get this blank line, I have to press Enter twice and get sent to a new block, which doesn't seem like the desired behavior to me.)

That's all the feedback I had here, this does seem to be working as intended in my testing.

youknowriad · 2017-04-17T08:46:10Z

Can we get some test cases for parsing HTML content into the structure for this block? Like blocks/api/test/parser.js

Yes, we could. I'm planing to do this in a separate PR because it implies some changes in how we register the blocks. I would like us to be able to test the block definition object without registering them. By this , I mean extracting the registerBlock call outside the block definition files (moving them to the parent `index.js) , that way, we could import the block definitions in unit tests without auto-registering them. I don't want this PR to be "polluted" by all these changes.

All the other feedback have been adressed

- Do not split when hitting Enter on the first row - Fix the nullable value error

youknowriad · 2017-04-18T08:43:42Z

@nylen Your fix is good since the block attributes are nullable by default. And I also fixed first line issue.

@aduth I've used something similar to #419 to avoid the div wrapper. But notice that I kept it in the save method for now because React don't support fragments yet. I applied the style to all the p tags.

jasmussen · 2017-04-18T08:51:00Z

Great discussion. CC: @mtias as I know you've thoughts about this.

mtias · 2017-04-18T16:25:47Z

It really seems we have two routes:

Different paragraphs are separate text blocks.
Contiguous paragraphs are part of the same text block.

In both cases a double (or single) enter would show the block-insert menu to the left. Depending on what you do next we would break out from the current text block or continue.

Given this could have significantly different UXs, what do you think, for the sake of moving faster, to make two different blocks for now:

A paragraph block (what Riad described).
A text block (multi-paragraph as the same block).

This would allow us to get a feel for them and make a better decision.

jasmussen · 2017-04-19T08:51:36Z

Matías put it pretty excellently, describing the fork in the road: paragraph as a block, or text as a continuous block.

If we can do both approaches — this PR being the Paragraph block approach — we will be able to test what works best in writing bouts. In the end, one approach may win, or we might decide that there is a place for both.

As Matías says, in both cases, double enter gives you access to the Inserter. Both at the end of content:

and between content:

In a paragraph-is-the-block world, double enter does split the block. But if you are writing in a text-is-the-block, the block isn't actually split until you make a choice in the inserter. If you choose to insert a paragraph here, the text block isn't split, whereas if you insert a gallery it does split.

@youknowriad just like how we are trying two parsers at the same time, can we do the same here? And if yes, should the continuous text block be a separate PR? If yes to both questions, it seems like this one should be good to merge.

youknowriad · 2017-04-19T08:55:09Z

@jasmussen Yes we could create another block for the Contiguous paragraphs are part of the same text block. approach. Even simpler than the two parser approaches because we could use the two blocks at the same time.

Contiguous paragraphs are part of the same text block. This means showing the block inserter in the middle of the text block which will give us a lot more headaches 😄

jasmussen · 2017-04-19T09:00:33Z

Contiguous paragraphs are part of the same text block. This means showing the block inserter in the middle of the text block which will give us a lot more headaches 😄

Is there anything we can do from an API or code design or user experience point of view that can make this headache less painful? What is the aspirin to this approach? If there's a design change I can make to simplify this, I'm totally open.

jasmussen · 2017-04-19T09:01:16Z

Yes we could create another block for the Contiguous paragraphs are part of the same text block. approach. Even simpler than the two parser approaches because we could use the two blocks at the same time.

👍 Awesome. Then seems worth doing as a separate PR. Want me to create a ticket for it?

youknowriad · 2017-04-19T09:04:19Z

Want me to create a ticket for it?

👍

Is there anything we can do from an API or code design or user experience point of view that can make this headache less painful?

Actually, I'm not sure we can do anything about it. It's just a hard task because we're mainly considering the Editable component (the TinyMCE wrapper) as a black box while this block means we'll need to figure when and how to get the inserter at the right place inside this component.

jasmussen · 2017-04-19T09:13:12Z

Created ticket here: #447

youknowriad · 2017-04-19T09:15:32Z

I'm merging this one for now, if you have any other concern, let me know I'll try to address it.

aduth · 2018-06-22T14:21:51Z

blocks/components/editable/index.js

@@ -58,6 +61,35 @@ export default class Editable extends wp.element.Component {
 		this.props.onChange( value );
 	}

+	onNewBlock() {


When is this function called? I cannot find any documentation for TinyMCE's NewBlock event. Based on its name, I would have thought it'd been called on a simple enter press, but in debugging this is not the case. Can we add a DocBlock explaining when this is expected to occur.

Having some memories about a hint from @iseulde from the TinyMCE source code. The idea was that each time a new paragraph is created the function is called or something like that :)

youknowriad added the Framework Issues related to broader framework topics, especially as it relates to javascript label Apr 12, 2017

youknowriad self-assigned this Apr 12, 2017

youknowriad requested a review from aduth April 12, 2017 10:58

youknowriad mentioned this pull request Apr 12, 2017

Inserter: Enable inserting a block using the inserter #407

Merged

aduth reviewed Apr 12, 2017

View reviewed changes

youknowriad force-pushed the add/insert-after branch from 49fd4ea to 4d9eaef Compare April 13, 2017 09:37

youknowriad commented Apr 13, 2017

View reviewed changes

joyously mentioned this pull request Apr 15, 2017

Nested blocks #428

Closed

youknowriad force-pushed the add/insert-after branch from 4d9eaef to e5c363e Compare April 17, 2017 08:16

youknowriad added 6 commits April 18, 2017 09:36

Original test strings

9a23d20

Using the create block helper

0dc010b

Fix the paragraph styling

706fbc5

Dropping unused dependency uuid

75a0f0d

Fix splitting behaviour when we have some formatting

49a169e

- No div wrapper for text block

de3265a

- Do not split when hitting Enter on the first row - Fix the nullable value error

youknowriad force-pushed the add/insert-after branch from 1139bc8 to de3265a Compare April 18, 2017 08:40

Using newBlock event instead of using setTimeout

fbdec2b

aduth mentioned this pull request Apr 18, 2017

Bump React to v16 alpha #445

Merged

Adding a comment about the temporary wrapper

ee12b37

jasmussen mentioned this pull request Apr 19, 2017

Add Block: Continuous Text #447

Closed

youknowriad merged commit 513bd1d into master Apr 19, 2017

youknowriad deleted the add/insert-after branch April 19, 2017 09:16

This was referenced Apr 19, 2017

Fix splitting in the middle of a text block #453

Merged

Add quote↔text block switching; add block switcher validation #465

Closed

nylen mentioned this pull request Jun 9, 2017

Try one block per paragraph #1078

Merged

aduth reviewed Jun 22, 2018

View reviewed changes

This was referenced Jun 27, 2018

Experiment: RichText: Format content from tree value #7583

Closed

Rich Text: Remove NewBlock event handling #7617

Merged

Editable: Enter splits the current paragraph into two blocks #409

Editable: Enter splits the current paragraph into two blocks #409

Conversation

youknowriad commented Apr 12, 2017 • edited Loading

nylen commented Apr 12, 2017

aduth commented Apr 12, 2017

youknowriad commented Apr 12, 2017

aduth commented Apr 12, 2017

jasmussen commented Apr 12, 2017 • edited Loading

youknowriad commented Apr 12, 2017

aduth commented Apr 12, 2017

youknowriad commented Apr 12, 2017

joyously commented Apr 12, 2017

ellatrix commented Apr 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nylen commented Apr 12, 2017

Layout blocks

Most common editing operations

nylen commented Apr 12, 2017

joyously commented Apr 12, 2017

youknowriad commented Apr 13, 2017 • edited Loading

Choose a reason for hiding this comment

nylen commented Apr 13, 2017

nylen commented Apr 13, 2017

youknowriad commented Apr 13, 2017

nylen commented Apr 13, 2017 • edited Loading

youknowriad commented Apr 17, 2017

youknowriad commented Apr 18, 2017

jasmussen commented Apr 18, 2017

mtias commented Apr 18, 2017

jasmussen commented Apr 19, 2017

youknowriad commented Apr 19, 2017 • edited Loading

jasmussen commented Apr 19, 2017

jasmussen commented Apr 19, 2017

youknowriad commented Apr 19, 2017

jasmussen commented Apr 19, 2017

youknowriad commented Apr 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad commented Apr 12, 2017 •

edited

Loading

jasmussen commented Apr 12, 2017 •

edited

Loading

youknowriad commented Apr 13, 2017 •

edited

Loading

nylen commented Apr 13, 2017 •

edited

Loading

youknowriad commented Apr 19, 2017 •

edited

Loading