-
Notifications
You must be signed in to change notification settings - Fork 3
Conversation
@dkonopka, could you check whether the |
WFM 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks fine.
I don't know why data is downcasted the way it is thoug, see the review comments for more details.
@MSGY I'd like to ask you to give it a quick testing. It is very similar to the horizontal rule (line) feature, but I'd still love to have another pair of eyes to check it.
@pomek Please make a 0.0.1 pre-release so that CI already kicks in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine now.
While doing 47135ef you didn't adjust the TC in https://github.com/ckeditor/ckeditor5-page-break/blob/a8b6d7b/tests/pagebreakui.js#L47 which caused tests to fail. I'll fix that as a part of R.
I also added a reference to your comment explaining putting span
inside a div
.
Suggested merge commit message (convention)
Feature: Initial implementation. Closes ckeditor/ckeditor5#1194.
Additional information
We need to improve the demo but it can be done outside this PR.