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

Fix list to paragraph transform #4460

Closed
wants to merge 1 commit into from
Closed

Conversation

ellatrix
Copy link
Member

Description

Currently nested lists fail to transform to paragraphs. This PR aims to fix that.

How Has This Been Tested?

Create list with indented items. Convert to paragraphs or quote.

@ellatrix ellatrix requested a review from youknowriad January 14, 2018 17:03
@youknowriad
Copy link
Contributor

I noticed a bug

screen shot 2018-01-15 at 08 53 33

I believe the children are not always an array.

@ellatrix
Copy link
Member Author

Whoops, looks like I corrected it in one place but not the other. :)

@ellatrix ellatrix force-pushed the fix/list-paragraph-transform branch from c831336 to 59063fb Compare January 15, 2018 10:41
@ellatrix
Copy link
Member Author

It's kind of messy when having to take prettier into account too. :/

@ellatrix ellatrix mentioned this pull request Jan 16, 2018
3 tasks
listContent.forEach( ( content, index ) => {
if ( content.type === 'ol' || content.type === 'ul' ) {
acc.push( ...listItemsToArray( castArray( get( content, 'props.children', [] ) ) ) );
acc.push( [] );
Copy link
Member

Choose a reason for hiding this comment

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

Is acc.push( [] ); required to have line breaks? maybe we specify in a comment the reason it is needed.

* @return {Array} Array of Editable value arrays.
*/
function listItemsToArray( items ) {
return items.reduce( ( acc, item ) => {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not totally certain but it looks like we may be able to use flatMap here and it has some performance advantages according to @aduth test cases. It may also simplify the code :)

Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

This is needing a rebase but generally the changes look good.

acc.push( ...listItemsToArray( castArray( get( content, 'props.children', [] ) ) ) );
acc.push( [] );
} else {
// No line breaks form prettier.
Copy link
Member

Choose a reason for hiding this comment

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

What does this comment mean? Should it be "from Prettier"? 🤷‍♂️

@ZebulanStanphill
Copy link
Member

Is this PR still relevant?

@ellatrix ellatrix closed this Aug 22, 2018
@ellatrix ellatrix deleted the fix/list-paragraph-transform branch August 22, 2018 12:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants