Skip to content
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

Try one block per paragraph #1078

Merged
merged 2 commits into from
Jun 15, 2017
Merged

Try one block per paragraph #1078

merged 2 commits into from
Jun 15, 2017

Conversation

ellatrix
Copy link
Member

@ellatrix ellatrix commented Jun 8, 2017

WIP. Tests will fail.

@ellatrix ellatrix force-pushed the try/single-p branch 2 times, most recently from f133040 to 4768e99 Compare June 8, 2017 12:48
@notnownikki
Copy link
Member

Nice, I'm liking this! Do you think it makes sense to make double enter in lists break out into a new text block? Might be a nice shortcut ,people are already used to doing that so they can carry on typing text after a list 😄

@nylen
Copy link
Member

nylen commented Jun 9, 2017

While this may simplify things from a technical perspective, I'm not seeing other benefits, and I think there are overwhelming reasons not to do it. So I am going to weigh in against an approach like this once again. Quoting from a significant portion of #409 (comment):

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 [because I think widgets will be one of the first areas to be replaced or upgraded in this way].

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.

More specifically, what I am referring to with "friction of navigating across blocks" is the typical text-editing operations described in #179, which we have so far decided not to support across blocks. In my opinion the only way to make a reasonable writing experience out of this is if a text block supports multiple paragraphs - many of the operations there are extremely common while writing sequences of text.

@ellatrix ellatrix force-pushed the try/single-p branch 3 times, most recently from c421508 to 83d2d2a Compare June 15, 2017 13:30
@ellatrix ellatrix requested a review from mtias June 15, 2017 14:01
@mtias mtias added [Feature] Block API API that allows to express the block paradigm. [Feature] Blocks Overall functionality of blocks labels Jun 15, 2017
const beforeFragment = beforeRange.extractContents();
const afterFragment = afterRange.extractContents();

const beforeReact = nodeListToReact( [ ...beforeFragment.childNodes ], createElement );
Copy link
Member

Choose a reason for hiding this comment

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

While not necessarily a guarantee, nodeListToReact will already handle the NodeList -> Array coercion:

https://github.com/iseulde/dom-react/blob/2159830/index.js#L116

if ( keyCode === ENTER && this.props.inline && this.props.onSplit ) {
const endNode = this.editor.selection.getEnd();

if ( endNode.nodeName !== 'BR' ) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we add some clarifying inline comments here? Not sure why we care to test whether the final node is a linebreak, or why we're expecting a second line break per the subsequent condition.

Edit: After testing, I understand now that we want two line breaks before split.

return Children.map( content, ( paragraph ) => (
cloneElement( paragraph, { style: { textAlign: align } } )
) );
return <p style={ { textAlign: align } }>{ content }</p>;
Copy link
Member

Choose a reason for hiding this comment

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

Outside scope, but given removal of element dependency, sooner than later I'd like us to move from inferred wp.element.createElement to explicit import { createElement } from 'element';. Related: #742

Copy link
Member Author

Choose a reason for hiding this comment

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

So I should add import { createElement } from 'element';?

Copy link
Member

Choose a reason for hiding this comment

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

Well, eventually, yes. For now since the pragma is defined as wp.element.createElement, it wouldn't be used anyways. Let's just leave it as-is for now, we can handle it separately.

@ellatrix ellatrix force-pushed the try/single-p branch 2 times, most recently from 1193996 to 3fd814e Compare June 15, 2017 16:09
@ellatrix
Copy link
Member Author

Rebased && addressed comments.

post-content.js Outdated
@@ -43,7 +46,11 @@ window._wpGutenbergPost = {
'<!-- /wp:core/heading -->',

'<!-- wp:core/text -->',
'<p>Imagine everything that WordPress can do is available to you quickly and in the same place on the interface. No need to figure out HTML tags, classes, or remember complicated shortcode syntax. That\'s the spirit behind the inserter—the <code>(+)</code> button you\'ll see around the editor—which allows you to browse all available content blocks and insert them into your post. Plugins and themes are able to register their own, opening up all sort of possibilities for rich editing and publishing.<p>Go give it a try, you may discover things WordPress can already insert into your posts that you didn\'t know about. Here\'s a short list of what you can currently find there:</p>',
'<p>Imagine everything that WordPress can do is available to you quickly and in the same place on the interface. No need to figure out HTML tags, classes, or remember complicated shortcode syntax. That\'s the spirit behind the inserter—the <code>(+)</code> button you\'ll see around the editor—which allows you to browse all available content blocks and insert them into your post. Plugins and themes are able to register their own, opening up all sort of possibilities for rich editing and publishing.',
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need a closing </p> here?

Copy link
Member Author

Choose a reason for hiding this comment

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

We do :)

@youknowriad
Copy link
Contributor

I kind of expected a single Enter to split the text block here, the br breaks in the text block feel weird to me.

@ellatrix
Copy link
Member Author

@youknowriad That's intended for the line break on enter and paragraph on double enter behaviour.

@ellatrix ellatrix force-pushed the try/single-p branch 2 times, most recently from 7ab61ea to fc694a7 Compare June 15, 2017 17:36
@mtias
Copy link
Member

mtias commented Jun 15, 2017

The single enter behaviour is something we need some early feedback on to make a proper call. It's working better than I expected from the multi-paragraph version, but we'd like to see whether it gets in the way of normal writing flow nonetheless.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Block API API that allows to express the block paradigm. [Feature] Blocks Overall functionality of blocks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants