-
Notifications
You must be signed in to change notification settings - Fork 3
Conversation
…ule to the editor.
…ontal-rule into t/ckeditor/1365
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.
Some organizational things first.
I fixed all reported issues. @dkonopka, feel free to remove |
|
||
ClassicEditor | ||
.create( document.querySelector( '#editor' ), { | ||
plugins: [ HorizontalRule, ... ], |
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.
Missing toolbar configuration. See https://ckeditor.com/docs/ckeditor5/latest/features/link.html#installation
Read more about {@link builds/guides/integration/installing-plugins installing plugins}. | ||
</info-box> | ||
|
||
## Contribute |
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.
Missing "Common API" section as in https://ckeditor.com/docs/ckeditor5/latest/features/link.html#common-api.
|
||
.ck-content .ck-horizontal-rule { | ||
/* Handle proper `<hr>` display next to elements with `float` property, e.g. side image case. */ | ||
overflow: hidden; |
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.
Is it ok that this style is only here? What about content produced by CKE5? Will hrs be trimmed there?
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.
cc @dkonopka
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.
theme/horizontalrule.css
Outdated
* For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license | ||
*/ | ||
|
||
.ck-content .ck-horizontal-rule { |
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.
This isn't a content style if it uses .ck-horizontal-rule
because this class is not printed out by the editor. So, this is part of UI styles and as such should, AFAIK, be prepended with .ck
instead.
cc @dkonopka
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.
You are completely right, thanks for being watchful 👍
theme/horizontalrule.css
Outdated
padding: 5px; | ||
} | ||
|
||
.ck-content .ck-horizontal-rule hr { |
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.
Similar as above – if these are supposed to be content styles then .ck-horizontal-rule
is just not there. So the only selector that we can use is .ck-content hr
.
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.
As commented.
Fixed. |
Suggested merge commit message (convention)
Feature: Initial implementation of the feature. Closes ckeditor/ckeditor5#1365.
Additional information
From the initial requirements, a few of them are not implemented in this PR:
After inserting the
hr
, the selection is set on the element.Pasting from Google Docs works but I didn't check how it goes with Word.
@dkonopka - could you say anything about?
Our demo isn't perfect but we can improve it after closing the PR.