Extracted nested editable styles and logic to widgets. #74

szymonkups · 2017-03-17T14:39:34Z

Suggested merge commit message (convention)

Fix: Nested editables have own CSS classes and their creation utility is extracted to widgets. Closes ckeditor/ckeditor5#5067.

BREAKING CHANGE: Widget utility function widgetize is renamed to toWidget.

Reinmar · 2017-03-20T12:20:10Z

src/widget/utils.js

+ * @param {module:engine/view/document~Document} viewDocument
+ * @returns {module:engine/view/editableelement~EditableElement}
+ */
+export function createNestedEditable( elementName, viewDocument ) {


It would be more consistent with widgetize() if it accepted an element.

BTW, it'd be hard to follow "widgetize"'s namig style in this case, so perhaps we could use these names: toWidget() and toWidgetEditable().

Reinmar · 2017-03-20T12:23:13Z

src/widget/utils.js

+
+	editable.on( 'change:isFocused', ( evt, property, is ) => {
+		if ( is ) {
+			editable.addClass( 'focused' );


Should be: ck-editable_focused.

See https://github.com/ckeditor/ckeditor5-design/wiki/Code-Style-Naming-Guidelines#css-classes

…le to toWidgetEditable.

…ilar parameter as toWidget function. Small docs fixes.

oleq

Look fine but requires some improvements in styles.

oleq · 2017-03-27T07:42:17Z

src/widget/utils.js

+ * @returns {module:engine/view/editableelement~EditableElement} Returns same element that was provided in `editable` param.
+ */
+export function toWidgetEditable( editable ) {
+	editable.setAttribute( 'contenteditable', 'true' );


I think this and the following line should be done via Template.apply(). One of the reasons we decided to have a Template class was that DOM attrs should be touched via unified API.

That's not true. This isn't UI. This is the engine and the element here isn't a native DOM element. We've been talking about unifying the APIs, but that never happened and I guess won't happen anytime soon.

Nice catch! I didn't notice that this isn't DOM. You're right, ofc ;-)

oleq · 2017-03-27T07:45:04Z

src/widget/utils.js

+	editable.setAttribute( 'contenteditable', 'true' );
+	editable.addClass( 'ck-editable' );
+
+	editable.on( 'change:isFocused', ( evt, property, is ) => {


new Template( { attributes: { contenteditable: true, class: [ 'ck-editable', bind.if( editable, 'isFocused', 'ck-editable_focused' ) ] } } ).apply( editable );

As above – nope. This is not a native element.

oleq · 2017-03-27T07:47:41Z

theme/imagecaption/theme.scss


 .ck-widget.image {
-	figcaption {
+	figcaption.ck-editable {


Does figcaption selector is really needed? Wouldn't .ck-editable be specific enough?

oleq · 2017-03-27T07:49:09Z

theme/widget/theme.scss

+@import '~@ckeditor/ckeditor5-theme-lark/theme/helpers/_states.scss';
+
+.ck-widget {
+	margin: 10px 0;


I think that https://github.com/ckeditor/ckeditor5-theme-lark/blob/master/theme/helpers/_spacing.scss#L32-L34 would make more sense.

oleq · 2017-03-27T07:55:12Z

theme/widget/theme.scss

+	padding: 0;
+
+	&.ck-widget_selected, &.ck-widget_selected:hover {
+		outline: 2px solid #ace;


These colors #ace, #ddd and yellow should be variables. Or better, they should come from https://github.com/ckeditor/ckeditor5-theme-lark/blob/master/theme/helpers/_colors.scss#L21-L27. Or even better, the widgets could extend the standard palette. We've never used it before, but it could still work:

@include ck-color-add( ( 'widget-selected': #ace, // 'widget-blurred': #ddd ) );

then

outline: 2px solid ck-color( 'widget-selected' );

BTW: We have focus color, which I think makes more sense than #ace.

oleq · 2017-03-27T07:56:10Z

theme/widget/theme.scss

+	}
+
+	&:hover {
+		outline: 2px solid yellow;


2px is a magic number. This should be a variable on the top of the file. Also: see above.

oleq · 2017-03-27T07:56:43Z

theme/widget/theme.scss

+	}
+
+	.ck-editable {
+		// The `:focus` styles is applied before `.focused` class inside editables.


Outdated docs (.focused?).

Also "styles is" vs. "styles are".

oleq · 2017-03-27T07:58:59Z

theme/widget/theme.scss

+	.ck-editable {
+		// The `:focus` styles is applied before `.focused` class inside editables.
+		// These styles show different border for a blink of an eye.
+		&:focus {


Despite the comment, I don't quite get why we need this structure :focus ..._focused. A few more words would explain it better. Or do we need it at all?

This can be simplified to:

.ck-editable { &.ck-editable_focused, &:focus { @include ck-focus-ring( 'outline' ); @include ck-box-shadow( $ck-inner-shadow ); background-color: ck-color( 'background' ); } }

oleq · 2017-03-27T07:59:37Z

theme/imagecaption/theme.scss


 .ck-widget.image {
-	figcaption {
+	figcaption.ck-editable {
 		background-color: ck-color( 'foreground' );
 		padding: 10px;


ck-spacing()

oleq · 2017-03-27T08:00:12Z

theme/imagecaption/theme.scss


 .ck-widget.image {
-	figcaption {
+	figcaption.ck-editable {
 		background-color: ck-color( 'foreground' );
 		padding: 10px;
 		font-size: .8em;


ck-font-size( -1 ) I guess.

Reinmar · 2017-03-28T09:34:10Z

theme/imagecaption/theme.scss

-@import '~@ckeditor/ckeditor5-theme-lark/theme/helpers/_shadow.scss';
-@import '~@ckeditor/ckeditor5-theme-lark/theme/helpers/_states.scss';
+@import '~@ckeditor/ckeditor5-theme-lark/theme/helpers/_spacing.scss';
+@import '~@ckeditor/ckeditor5-theme-lark/theme/helpers/_fonts.scss';

 .ck-widget.image {


It'd be better if these styles had nothing to do with the widget classes. The widgets theme use widget classes (and hence applies to the editor only) and the image theme uses image classes.

Actually, this is a bigger topic. For the widgets we're defining styles which will apply in a context of the editor cause we use editor classes like .ck-widget. For the content (like figure.image) we want to provide generic content styles like in the CKEditor 4's contents.css. So, they need to be overrideable but still so specific to apply to editor contents only, not the whole page.

The only idea I've got now is to prefix all of them with .ck-editor__editable. WDYT, @oleq?

Isn't .ck-editor .image enough? Because I think it is and it's a better DX because devs don't need to think what is "editable" and why. All they want is a styled image in the CKEditor.

Is .ck-editor applied in the inline editor? It'd be best if the class was as short as possible, but I think it can't be .ck-editor.

If we would like to use something shorter than .ck-editor__editable, then perhaps we can start using an additional class .ck-content for the editables?

Is .ck-editor applied in the inline editor?

No. A good point. Let's proceed with .ck-editor__editable then.

Reported it for the future: https://github.com/ckeditor/ckeditor5-ui/issues/176.

Reinmar · 2017-03-28T13:14:22Z

theme/widget/theme.scss

+
+	&:hover {
+		outline: $widget-outline-thickness solid ck-color( 'widget-hover' );
+	}


Something's wrong here... Indentation! :P

Extracted nested editable styles and logic to widgets.

b0b4bc7

szymonkups requested a review from Reinmar March 17, 2017 14:39

Reinmar reviewed Mar 20, 2017

View reviewed changes

szymonkups added 2 commits March 20, 2017 14:54

Renamed utils functions: widgetize to toWidget and createNestedEditab…

a80dff3

…le to toWidgetEditable.

CSS classes name refactoring in widgets.

7ef4895

szymonkups force-pushed the t/57 branch from 66319f3 to 4a7df47 Compare March 21, 2017 09:34

Widget util function toWidgetEditable now accepts element to have sim…

a426d5a

…ilar parameter as toWidget function. Small docs fixes.

szymonkups force-pushed the t/57 branch from 4a7df47 to a426d5a Compare March 21, 2017 09:35

szymonkups requested a review from oleq March 21, 2017 09:57

szymonkups mentioned this pull request Mar 23, 2017

T/77 Image captions in the view are hidden instead of being removed #81

Merged

oleq suggested changes Mar 27, 2017

View reviewed changes

oleq added the status:review- label Mar 27, 2017

Fixed widget and caption styles.

3be8718

szymonkups removed the status:review- label Mar 27, 2017

Reinmar reviewed Mar 28, 2017

View reviewed changes

Added ck-editor__editable prefix to content styles.

aeb276b

Reinmar reviewed Mar 28, 2017

View reviewed changes

szymonkups and others added 2 commits March 28, 2017 15:16

Fixed indentation in theme file.

c025e49

Use data classes to style the content. Removed invalid comment.

d797993

Reinmar merged commit ecbe435 into master Mar 28, 2017

Reinmar deleted the t/57 branch March 28, 2017 14:02

Reinmar mentioned this pull request Oct 9, 2019

Mark each root editable with ".ck-content" class ckeditor/ckeditor5#5323

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extracted nested editable styles and logic to widgets. #74

Extracted nested editable styles and logic to widgets. #74

szymonkups commented Mar 17, 2017 •

edited by pomek

Loading

Reinmar Mar 20, 2017

Reinmar Mar 20, 2017

oleq left a comment

oleq Mar 27, 2017

Reinmar Mar 27, 2017

oleq Mar 27, 2017

oleq Mar 27, 2017

Reinmar Mar 27, 2017

oleq Mar 27, 2017 •

edited

Loading

oleq Mar 27, 2017

oleq Mar 27, 2017

oleq Mar 27, 2017

oleq Mar 27, 2017

oleq Mar 27, 2017

szymonkups Mar 27, 2017

oleq Mar 27, 2017

oleq Mar 27, 2017

Reinmar Mar 28, 2017

oleq Mar 28, 2017

Reinmar Mar 28, 2017 •

edited

Loading

oleq Mar 28, 2017

Reinmar Mar 28, 2017

Reinmar Mar 28, 2017

szymonkups Mar 28, 2017

Extracted nested editable styles and logic to widgets. #74

Extracted nested editable styles and logic to widgets. #74

Conversation

szymonkups commented Mar 17, 2017 • edited by pomek Loading

Suggested merge commit message (convention)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleq Mar 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Reinmar Mar 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szymonkups commented Mar 17, 2017 •

edited by pomek

Loading

oleq Mar 27, 2017 •

edited

Loading

Reinmar Mar 28, 2017 •

edited

Loading