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

Fixed paste bugs on headings and lists #3949

Merged
merged 3 commits into from Dec 18, 2017

Conversation

Projects
None yet
4 participants
@jorgefilipecosta
Member

jorgefilipecosta commented Dec 12, 2017

Description

This PR aims to fix #3241, #3726, and #3351. Affecting paste on headings and list blocks.
It proposes an alternative approach to #3842, this alternative is generic and requires very low effort from block creators.

The idea behind this PR is that our definition of what HTML elements are inline elements should not be static. We should allow blocks to set HTML elements they would like to be treated as inline elements.
So list block can set list elements to be interpreted as inline, this way when we paste a list into an empty list block we will not create a new list as it happens right now. Also when we paste list items in the middle of list block we "merge" the items as it would be expected.

This PR misses the addition of test cases and updates in the documentation, they will be added as soon as we are more confident we can follow this approach.

How Has This Been Tested?

Paste an HTML list in an empty list block see no new list was created and the list was correctly pasted.
Paste an HTML list in the middle of a list block see the list was correctly merged.
Paste an HTML heading block in an empty heading block see no new headings were created.
Paste an HTML heading block in a non-empty heading block see contents were correctly merged.

After Screenshots:

Empty list paste

dec-12-2017 13-27-15

Non-Empty list paste

dec-12-2017 13-22-41

Heading paste

dec-12-2017 13-29-55

Before Screenshots:

Empty list paste

dec-12-2017 13-26-21

Non-Empty list paste

dec-12-2017 13-24-27

Heading paste

dec-12-2017 13-30-40

@mcsf

This comment has been minimized.

Show comment
Hide comment
@mcsf

mcsf Dec 12, 2017

Contributor

The author-facing benefit of this change is very clear, and welcome. However, could we go further and let Editable infer more? The required tagName prop seems like a pretty good hint:

  • For Heading, tagName is expected to be one of [ 'h1', 'h2', 'h3', 'h4', 'h5', 'h6' ], which exactly fits its additionalInlineNodes prop.
  • For List, tagName is expected to be one of [ 'ol', 'ul' ], which is almost the same as its additionalInlineNodes prop, save for the missing 'li'. Still pretty close.

Maybe there are use cases out there that warrant a fully customizable prop like additionalInlineNodes, but is it worth exploring a more autonomous Editable?

Contributor

mcsf commented Dec 12, 2017

The author-facing benefit of this change is very clear, and welcome. However, could we go further and let Editable infer more? The required tagName prop seems like a pretty good hint:

  • For Heading, tagName is expected to be one of [ 'h1', 'h2', 'h3', 'h4', 'h5', 'h6' ], which exactly fits its additionalInlineNodes prop.
  • For List, tagName is expected to be one of [ 'ol', 'ul' ], which is almost the same as its additionalInlineNodes prop, save for the missing 'li'. Still pretty close.

Maybe there are use cases out there that warrant a fully customizable prop like additionalInlineNodes, but is it worth exploring a more autonomous Editable?

@iseulde

This comment has been minimized.

Show comment
Hide comment
@iseulde

iseulde Dec 13, 2017

Member

I'm liking how this fixes the list problem as well!

I find additionalInlineNodes a bit poorly named as the tags that will be supplied are not inline tags/nodes at all. The name should communicate that the presence of these tags alone will not trigger automatically trigger block mode for paste, which is hard to describe in a name... Maybe something like pasteAutoInlineModeTags?

@mcsf Not sure how we could get rid of this prop by looking at the Editable tagName?

Member

iseulde commented Dec 13, 2017

I'm liking how this fixes the list problem as well!

I find additionalInlineNodes a bit poorly named as the tags that will be supplied are not inline tags/nodes at all. The name should communicate that the presence of these tags alone will not trigger automatically trigger block mode for paste, which is hard to describe in a name... Maybe something like pasteAutoInlineModeTags?

@mcsf Not sure how we could get rid of this prop by looking at the Editable tagName?

Show outdated Hide outdated blocks/editable/index.js Outdated
@mcsf

This comment has been minimized.

Show comment
Hide comment
@mcsf

mcsf Dec 13, 2017

Contributor

@mcsf Not sure how we could get rid of this prop by looking at the Editable tagName?

@iseulde I was thinking of something along the lines of what the branch now looks like; does that answer your question?

Contributor

mcsf commented Dec 13, 2017

@mcsf Not sure how we could get rid of this prop by looking at the Editable tagName?

@iseulde I was thinking of something along the lines of what the branch now looks like; does that answer your question?

@iseulde

This comment has been minimized.

Show comment
Hide comment
@iseulde

iseulde Dec 13, 2017

Member

@mcsf Yes :)

Member

iseulde commented Dec 13, 2017

@mcsf Yes :)

Show outdated Hide outdated blocks/api/raw-handling/index.js Outdated
Show outdated Hide outdated blocks/api/raw-handling/utils.js Outdated
Show outdated Hide outdated blocks/api/raw-handling/utils.js Outdated
Show outdated Hide outdated blocks/api/raw-handling/utils.js Outdated
Show outdated Hide outdated blocks/api/raw-handling/utils.js Outdated
} );
it( 'should not be inline content', () => {
equal( isInlineContent( '<div>test</div>' ), false );
equal( isInlineContent( '<em>test</em><div>test</div>' ), false );
equal( isInlineContent( 'test<br><br>test' ), false );
equal( isInlineContent( '<em><div>test</div></em>' ), false );
equal( isInlineContent( '<li>test</li>', 'p' ), false );

This comment has been minimized.

@mcsf

mcsf Dec 14, 2017

Contributor

I suggest adding a test that mixes the list group with the header group.

@mcsf

mcsf Dec 14, 2017

Contributor

I suggest adding a test that mixes the list group with the header group.

Show outdated Hide outdated blocks/api/raw-handling/index.js Outdated

jorgefilipecosta added some commits Dec 12, 2017

Implemented isInlineForTag mechanism
This mechanism allows treating elements as inline if they are being pasted in specific tags. So for example in heading blocks we can interpret headings as inline content and on lists we can interpret lists as inline content.
Added tag groups for headings and lists to the inlineWhitelistTagGrou…
…ps mechanism.

This improves the behaviour of paste in already existing list and heading blocks.
@jorgefilipecosta

This comment has been minimized.

Show comment
Hide comment
@jorgefilipecosta

jorgefilipecosta Dec 18, 2017

Member

Hi @mcsf thank you for reviewing in detail this PR, it was updated.

Member

jorgefilipecosta commented Dec 18, 2017

Hi @mcsf thank you for reviewing in detail this PR, it was updated.

@mcsf

mcsf approved these changes Dec 18, 2017

@jorgefilipecosta jorgefilipecosta merged commit f181a08 into master Dec 18, 2017

3 checks passed

codecov/project 40.72% (+2.06%) compared to 227a93f
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@jorgefilipecosta jorgefilipecosta deleted the fix/fixed-paste-bugs-on-headings-lists branch Dec 18, 2017

@mtias

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Jan 1, 2018

Contributor

@jorgefilipecosta @iseulde @mcsf not sure when we regressed on this, but we can no longer paste a list like:

* One
* Two
* Three

and have it converted into a proper list.

Contributor

mtias commented Jan 1, 2018

@jorgefilipecosta @iseulde @mcsf not sure when we regressed on this, but we can no longer paste a list like:

* One
* Two
* Three

and have it converted into a proper list.

@iseulde

This comment has been minimized.

Show comment
Hide comment
@iseulde

iseulde Jan 2, 2018

Member

@mtias This probably never worked with the list on its own. If there are no two consecutive line breaks detected, MarkDown parsing will be skipped to make sure inline paste works correctly. I'll attempt to fix this right now. Maybe we can just check for a single line break or try to get rid of this check entirely.

Member

iseulde commented Jan 2, 2018

@mtias This probably never worked with the list on its own. If there are no two consecutive line breaks detected, MarkDown parsing will be skipped to make sure inline paste works correctly. I'll attempt to fix this right now. Maybe we can just check for a single line break or try to get rid of this check entirely.

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