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

Support for nested indentation on pasted content #94

Merged
merged 9 commits into from
Mar 5, 2020
Merged

Support for nested indentation on pasted content #94

merged 9 commits into from
Mar 5, 2020

Conversation

gjhenrique
Copy link
Contributor

Suggested merge commit message (convention)

Feature: Stand on the shoulder of the giant @f1ames to support lists indentation in pasted content added.
Closes ckeditor/ckeditor5#2518


Additional information

We're currently using ckeditor in one of our projects and it's a worderfull library. Thank you for open sourcing it.

Everything works fine, except the support for nested lists is a requirement. I checked that #53 was pending because of the normalization of some items. This PR tries to fix that normalization.

I rebased to master from the existing branch, fixed the conflicts and modified the required code to support it. On top of that, I commited fca9d77 to support the correct normalization.

Lemme know if something is missing

\cc
@Mgsy @Reinmar

@coveralls
Copy link

coveralls commented Feb 26, 2020

Pull Request Test Coverage Report for Build 368

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 367: 0.0%
Covered Lines: 235
Relevant Lines: 235

💛 - Coveralls

@gjhenrique gjhenrique changed the title Nested list reborn Support for nested indentation on pasted content Feb 26, 2020
@gjhenrique
Copy link
Contributor Author

I just pushed 9229155 to fix a filed test build related to the removal of <o:p> introduced on #81

@Reinmar
Copy link
Member

Reinmar commented Feb 27, 2020

Wow :) You're a hero already! :)

@ckeditor/qa-team, could you check whether this PR doesn't break something that perhaps worked already? If there are no regressions, even if this PR doesn't handle some things perfectly, I'm all for merging it.

@mlewand, I leave the code review to you. Similarly to the QA aspect – as long as there are no major issues, let's merge this.

Copy link
Member

@FilipTokarski FilipTokarski left a comment

Choose a reason for hiding this comment

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

Looks fine to me

Copy link
Member

@Mgsy Mgsy left a comment

Choose a reason for hiding this comment

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

LGTM.

@laertispappas
Copy link

Hi guys would it be possible to have this changeset released? thank you in advance.

@Reinmar
Copy link
Member

Reinmar commented Mar 3, 2020

@mlewand, there's a conflict due to changes done by @pomek recently. It should be fairly easy to resolve.

@laertispappas, we need to review the code first and once it's done and looks fine, we'll merge the PR. It will be then included in the next release.

@mlewand
Copy link
Contributor

mlewand commented Mar 3, 2020

@Reinmar  Sure, I'll take a look at it tomorrow 👍

@gjhenrique
Copy link
Contributor Author

@mlewand
I took the liberty to rebase from master and fix the conflict. I hope that's fine.

Copy link
Contributor

@mlewand mlewand left a comment

Choose a reason for hiding this comment

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

Generally looks fine, good job 👍 I can see that you were able to even make the code a little more concise.

That being said new parts will surely have to be adjusted while fixing ckeditor/ckeditor5#6380 - good to see we're closer to that!

@mlewand mlewand merged commit 58ae829 into ckeditor:master Mar 5, 2020
@Reinmar
Copy link
Member

Reinmar commented Mar 10, 2020

Awesome news :) Thanks @gjhenrique! 👏

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