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

Paste: Treat single-item lists as paragraphs #5354

Closed
wants to merge 1 commit into from

Conversation

mcsf
Copy link
Contributor

@mcsf mcsf commented Mar 2, 2018

Description

When pasting content from rich-text sources, if the object being pasted is a single item from a list, treat it as a paragraph. That is, pasting the object will not result in a new List block with a single item, but rather a Paragraph block with the text content copied from the source.

gutenberg-single-item-list-paste

How Has This Been Tested?

  • Own tests for the new transformer.
  • Integration tests for blocks/api/raw-handler
  • Manual testing — see screencast above. Try the same but with some formatting, e.g. bold in the list items.

Types of changes

Enhancement. This is an opinionated optimization for something that has emerged as a common enough pattern.

Checklist:

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

@mcsf mcsf added this to the 2.3 milestone Mar 2, 2018
@aduth
Copy link
Member

aduth commented Mar 2, 2018

Personally, I don't find this to be the expected behavior.

Even within Google Docs, when copying the single list item from one document to another, its list-iness is preserved.

@@ -137,6 +141,7 @@ export default function rawHandler( { HTML, plainText = '', mode = 'AUTO', tagNa
] ) );

piece = deepFilterHTML( piece, [
singleItemListConverter,
Copy link
Member

@jorgefilipecosta jorgefilipecosta Mar 2, 2018

Choose a reason for hiding this comment

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

I think the "filter" was implemented in separate so the changes are isolated, but as we already have a listReducer filter would it make sense for singleItemListConverter to be part of the other filter?

Not related to this changes but we have 3 consecutive calls to deepFilterHTML, Should we just join them all? One has a comment specifying it should be the first but even if we join them all we can still have comment saying msListConverter should be the first filter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't mind moving it out of that particular deepFilterHTML call, but differences in the document mean that the ms-word-online integration test fails if the filter is placed in the same deepFilterHTML call as listReducer, …, inlineContentConverter.

That said, I agree there's room to improve the overall efficiency in that bit of rawHandler.

@mcsf
Copy link
Contributor Author

mcsf commented Mar 2, 2018

Even within Google Docs, when copying the single list item from one document to another, its list-iness is preserved.

Fair enough.

For what it's worth, currently neither this branch nor master are very helpful when pasting a single-item list onto an existing list:

Before After
gutenberg-paste-single-item-list-before gutenberg-paste-single-item-list-after

I may have a fix for this, but since it will affect all splitting pastes in RichText, it needs testing. For this, I'll move this one out of the milestone.

@mcsf mcsf modified the milestones: 2.3, 2.4 Mar 2, 2018
@jorgefilipecosta
Copy link
Member

jorgefilipecosta commented Mar 2, 2018

Not judging if this should be the right behavior or not, this seems to work fine. I wonder if for this behavior, interpreting simple item paste as plaintext would also not have some advantages. E.g: if I paste a single list item in an heading it would still be a heading.
@mcsf in both this branch and master when I paste a list with one item into a list it is correctly pasted as a list item
mar-02-2018 17-12-35

Maybe the list item you are pasting from has a more complex markup?

@mcsf
Copy link
Contributor Author

mcsf commented Mar 2, 2018

I wonder if for this behavior, interpreting simple item paste as plaintext would also not have some advantages

The problem with this is losing rich-text formatting from the source.

Maybe the list item you are pasting from has a more complex markup?

Yes, I'm using Google Docs, which is a little idiosyncratic.

@ellatrix
Copy link
Member

I have to agree with @aduth that I'm personally expecting the current behaviour... Plus it's introducing quite a bit of complex, list-specific code. In my opinion this is a bit too much correcting.

Pasting a list item into an existing list and it creating empty items should be fixed though.

@aduth aduth modified the milestones: 2.4, 2.5 Mar 14, 2018
@mcsf
Copy link
Contributor Author

mcsf commented Mar 21, 2018

Thanks for the feedback, everyone. I'm a bit divided on this one. Since we had chats about this long ago, I'd like @mtias's input on this one. Until then, we can keep it closed. The list-pasting inconsistencies are now tracked in #5727.

@mcsf mcsf closed this Mar 21, 2018
@mcsf mcsf deleted the add/raw-handler-single-item-list-converter branch March 21, 2018 13:42
@mtias
Copy link
Member

mtias commented May 2, 2018

@aduth @iseulde @mcsf @jorgefilipecosta I think we need to revisit this a bit. I agree pasting a single item in an empty paragraph we can respect the source and create a list. But this behaviour makes it really hard to copy text and be able to paste it in a heading or list without messing things up and creating a separate block.

Here's my proposal:

  • If you are on an empty paragraph, respect the source.
  • If you copied more than one line, respect the source.
  • If you copy a single line, paste as plain text.

@mcsf
Copy link
Contributor Author

mcsf commented May 3, 2018

Tracked in #6555 to keep this closed PR clean.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants