-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Conversation
f133040
to
4768e99
Compare
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 😄 |
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):
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. |
c421508
to
83d2d2a
Compare
blocks/editable/index.js
Outdated
const beforeFragment = beforeRange.extractContents(); | ||
const afterFragment = afterRange.extractContents(); | ||
|
||
const beforeReact = nodeListToReact( [ ...beforeFragment.childNodes ], createElement ); |
There was a problem hiding this comment.
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
blocks/editable/index.js
Outdated
if ( keyCode === ENTER && this.props.inline && this.props.onSplit ) { | ||
const endNode = this.editor.selection.getEnd(); | ||
|
||
if ( endNode.nodeName !== 'BR' ) { |
There was a problem hiding this comment.
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>; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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';
?
There was a problem hiding this comment.
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.
1193996
to
3fd814e
Compare
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.', |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do :)
I kind of expected a single Enter to split the text block here, the |
@youknowriad That's intended for the line break on enter and paragraph on double enter behaviour. |
7ab61ea
to
fc694a7
Compare
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. |
WIP. Tests will fail.