Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

New lines pasted as a plain text will always create a new paragraph #64

Merged
merged 4 commits into from
Jun 26, 2019

Conversation

msamsel
Copy link
Contributor

@msamsel msamsel commented May 31, 2019

Suggested merge commit message (convention)

Other: New lines pasted as a plain text will always create a new paragraph. Closes ckeditor/ckeditor5#1727.

BREAKING CHANGE: From now every new line pasted in editor as a plain text, will create a new paragraph. Read more at ckeditor/ckeditor5#1727.

@jodator jodator self-requested a review June 5, 2019 08:22
Copy link
Contributor

@jodator jodator left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -10,21 +10,12 @@ describe( 'plainTextToHtml()', () => {
expect( plainTextToHtml( 'x y <z>' ) ).to.equal( 'x y &lt;z&gt;' );
} );

it( 'turns double line breaks into paragraphs', () => {
Copy link
Member

Choose a reason for hiding this comment

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

It's usually good to leave all the tests of the previous implementation. Right now, we know that a single line break turns into a paragraph. But what about two? This is still a pretty distinctive case. Also, since prior to your changes there was a code which handled double line breaks, how do you know now that it's completely removed? There's actually a non-theoretical chance that that code remained there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Reinmar case with 1, 2, 3 new lines is tested in next test "turns combination of different amount of line breaks to paragraphs":
a\nb\n\nc\n\n\nd

Copy link
Member

Choose a reason for hiding this comment

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

The purpose of the last test is to see if a varying number of line breaks doesn't e.g. cause some tags to be left open.

@@ -10,21 +10,12 @@ describe( 'plainTextToHtml()', () => {
expect( plainTextToHtml( 'x y <z>' ) ).to.equal( 'x y &lt;z&gt;' );
} );

it( 'turns double line breaks into paragraphs', () => {
expect( plainTextToHtml( 'x\n\ny\n\nz' ) ).to.equal( '<p>x</p><p>y</p><p>z</p>' );
it( 'turns single line breaks into paragraphs', () => {
Copy link
Member

Choose a reason for hiding this comment

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

BTW, it's "single line break", not "single line breaks".

@Reinmar Reinmar merged commit f0eb3a0 into master Jun 26, 2019
@Reinmar Reinmar deleted the t/ckeditor5/1727 branch June 26, 2019 09:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pasted plain text with line breaks, can't add a list to each line
3 participants