List: Convert single paragraph into one item per line #9162

mcsf · 2018-08-20T13:08:56Z

Description

When converting a single paragraph (i.e., when not in multi-block selection) to a list, look for line breaks (<br>) in the paragraph and yield one list item per paragraph line.

How has this been tested?

Test case, and manual testing. See screenshot. Make sure that:

Converting a single paragraph with line breaks yields multiple items in a new list.
Converting a single paragraph with no line breaks works as before.
Converting multiple paragraphs works as before.

Screenshots

Types of changes

I wrote splitOnLineBreak as a single file inside the List block's source directory to make it easier to test.
I didn't place it in a utils module because I don't necessarily want this to be a public module to manipulate rich text.

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.

When converting a single paragraph (i.e., when not in multi-block selection) to a list, look for line breaks (`<br>`) in the paragraph and yield one list item per paragraph line.

mtias · 2018-08-20T13:24:52Z

This seems a great small improvement to the experience.

jorgefilipecosta · 2018-08-20T17:04:44Z

packages/block-library/src/list/split-on-line-break.js

+ * array of strings and element-like objects (e.g. `{ type: strong, props: {
+ * children: […] } }`).
+ *
+ * @param {Array}    fragments Rich-text content


Minor: Should we be more specific and document that this function receives a WPBlockChildren parameter?

Ah, that's a change I missed — good to know. :)

jorgefilipecosta · 2018-08-20T17:08:44Z

packages/block-library/src/list/split-on-line-break.js

+ * @param {Array}    fragments Rich-text content
+ * @return {Array}   Array of rich-text content
+ */
+export default function splitOnLineBreak( fragments ) {


I think we have the objective of encapsulating details about the children object (it has a type, it has props etc...) inside packages/blocks/src/api/children.js. Would it make sense to implement this function there?

Hm, I had left this note in the PR description:

I wrote splitOnLineBreak as a single file inside the List block's source directory to make it easier to test.

I didn't place it in a utils module because I don't necessarily want this to be a public module to manipulate rich text.

Does it make sense to you? Should we later revise what we'd like to exist in api/children?

Yes it makes sense, we have the downside of accessing the children object directly but I don't see a better alternative.
The alternative I see would be using toHTML to convert the children to HTML manipulate the HTML and then convert it back to a children object using fromDOM, but that seems like an unnecessary complexity and would make the code much more complex.
I think give the situation accessing the props of the children object here seems to be the best option. Maybe in the future, we can think of moving the function to api/children or adding abstractions in api/children that make this function possible.

mcsf · 2018-08-20T17:15:19Z

packages/block-library/src/list/split-on-line-break.js

+
+		// If we've just skipped a line break, append the
+		// next fragment as a new item.
+		const prevFragment = i > 0 && arr[ i - 1 ];


TODO for myself: use of arr is vestigial, I'll probably just rewrite as fragments

aduth · 2018-08-20T17:40:05Z

packages/block-library/src/list/split-on-line-break.js

+		// If we've just skipped a line break, append the
+		// next fragment as a new item.
+		const prevFragment = i > 0 && arr[ i - 1 ];
+		if ( prevFragment && prevFragment.type === 'br' ) {


Observation: We're piercing the abstraction of the node / children type here.

Still an open question as to whether that's okay / expected. But it does further solidify us into committing to this object shape.

@aduth, before I add any comments to it, does 43ad834 look like a better compromise?

I like the direction. We should keep the vocabulary of "node" for the singular form, in place of isChildOfType (in node.js).

Cool. Rectified in 8af5cc3.

jorgefilipecosta

This seems to be working correctly on my tests 👍
And looks like a nice enhancement to me.

I detect a detail that looked strange at first sight. If we write 'aaaabbbb' make the text bold and then we add a new line in the middle when converting we get a list with one item aaaa
bbbb.
If we write aaaabbbb, add new line and we make the test bold we get a list with two items aaaa, bbbb. We get two different conversions for results which look the same, but I checked this difference is caused by the HTML code generated being different and is expected.

youknowriad · 2018-08-20T18:49:48Z

Just so you know, The transform was built this way originally, but it was simplified to create a unique list item per paragraph as part of the "multiselection" transform introduction.

The idea is that if you transform multiple paragraphs to a list and you transform back to paragraphs, you get the exact same output.

mcsf · 2018-08-21T14:02:32Z

Thanks for the context, @youknowriad.

The idea is that if you transform multiple paragraphs to a list and you transform back to paragraphs, you get the exact same output.

I normally agree with this sort of consistency, but I see the point of optimizing for "doing the convenient thing at the right time" for certain micro-interactions, and this seems like an overall win, especially when dealing with non-native content (e.g. pre-Gutenberg content, or non-block content pasted from remote sources).

Thanks for the review, @jorgefilipecosta!

mcsf · 2018-08-21T22:11:09Z

Thanks for all the feedback. I think we can get this in.

List: Convert single paragraph into one item per line

a7a19fb

When converting a single paragraph (i.e., when not in multi-block selection) to a list, look for line breaks (`<br>`) in the paragraph and yield one list item per paragraph line.

mcsf added [Feature] Blocks Overall functionality of blocks [Feature] Block Transforms Block transforms from one block to another labels Aug 20, 2018

mcsf requested a review from mtias August 20, 2018 13:08

mcsf added the [Feature] Writing Flow Block selection, navigation, splitting, merging, deletion... label Aug 20, 2018

mcsf requested a review from jorgefilipecosta August 20, 2018 16:33

jorgefilipecosta reviewed Aug 20, 2018

View reviewed changes

Update jsdoc to mention WPBlockChildren

7b4ceac

mcsf commented Aug 20, 2018

View reviewed changes

mtias added this to the 3.7 milestone Aug 20, 2018

aduth reviewed Aug 20, 2018

View reviewed changes

jorgefilipecosta approved these changes Aug 20, 2018

View reviewed changes

mcsf added 3 commits August 21, 2018 15:03

(minor) Drop last reduce argument

1b85c24

Try minimally extending children API

43ad834

Adopt WPBlockNode nomenclature, move isNodeOfType to node API

8af5cc3

mcsf merged commit a5c89c7 into master Aug 21, 2018

mcsf deleted the update/single-paragraph-to-multi-item-list branch August 21, 2018 22:11

0aveRyan mentioned this pull request Aug 24, 2018

Add Code → Preformatted block transform #9326

Merged

List: Convert single paragraph into one item per line #9162

List: Convert single paragraph into one item per line #9162

Uh oh!

Conversation

mcsf commented Aug 20, 2018

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

Uh oh!

mtias commented Aug 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jorgefilipecosta left a comment

Choose a reason for hiding this comment

Uh oh!

youknowriad commented Aug 20, 2018

Uh oh!

mcsf commented Aug 21, 2018

Uh oh!

mcsf commented Aug 21, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants