Styles system for Easy Image #1518

Comandeer · 2018-01-29T15:14:06Z

What is the purpose of this pull request?

New feature

Does your PR contain necessary tests?

All patches which change the editor code must include tests. You can always read more
on PR testing,
how to set the testing environment and
how to create tests
in the official CKEditor documentation.

This PR contains

Unit tests
Manual tests

What changes did you make?

Rewrite of whole commands system for EI, enabling to add custom styles. Additionaly existing commands (easyimageFull and easyimageSide) were moved into the new style system.

mlewand

I just checked docs, and test briefly, there are a few things that needs to be addressed before I proceed with review further.

File names for icons got long and tricky, there's easyimagealigncenter.png, easyimagealignleft.png and easyimagealignright.png. Since we already have easyimageside.png, how about making it a easyimagecenter.png (skip "align" part)? Simpler and more consistent.

mlewand · 2018-01-31T14:21:38Z

tests/plugins/easyimage/manual/customstyles.md

+@bender-ckeditor-plugins: sourcearea, wysiwygarea, floatingspace, toolbar, easyimage
+
+1. Click on image widget.
+2. Click on one of button in balloon toolbar. **Note:** first style is a totally custom one, therefore it does not have any icon.


(…) one of buttons in the toolbar. (…)

mlewand · 2018-01-31T14:28:34Z

tests/plugins/easyimage/manual/customstyles.html

+	<p>Armstrong spent about <s>three and a half</s> two and a half hours outside the spacecraft, Aldrin slightly less; and together they collected 47.5 pounds (21.5&nbsp;kg) of lunar material for return to Earth. A third member of the mission, <a href="http://en.wikipedia.org/wiki/Michael_Collins_(astronaut)">Michael Collins</a>, piloted the <a href="http://en.wikipedia.org/wiki/Apollo_Command/Service_Module">command</a> spacecraft alone in lunar orbit until Armstrong and Aldrin returned to it for the trip back to Earth.</p>
+</div>
+
+<h2>Inline editor</h2>


There's no source button in the inline editor, so let's skip it and leave just divarea.

mlewand · 2018-01-31T14:35:12Z

tests/plugins/easyimage/manual/customstyles.html

@@ -0,0 +1,53 @@
+<h2>Classic editor</h2>
+<div id="classic">
+	<p><strong>Apollo 11</strong> was the spaceflight that landed the first humans, Americans <a href="http://en.wikipedia.org/wiki/Neil_Armstrong">Neil Armstrong</a> and <a href="http://en.wikipedia.org/wiki/Buzz_Aldrin">Buzz Aldrin</a>, on the Moon on July 20, 1969, at 20:18 UTC. Armstrong became the first to step onto the lunar surface 6 hours later on July 21 at 02:56 UTC.</p>


Do we actually need all the wrapping text? It's a lot of it. Try to reduce it.

mlewand · 2018-01-31T14:35:20Z

tests/plugins/easyimage/manual/customtoolbar.html

@@ -0,0 +1,43 @@
+<h2>Classic editor</h2>
+<div id="classic">
+	<p><strong>Apollo 11</strong> was the spaceflight that landed the first humans, Americans <a href="http://en.wikipedia.org/wiki/Neil_Armstrong">Neil Armstrong</a> and <a href="http://en.wikipedia.org/wiki/Buzz_Aldrin">Buzz Aldrin</a>, on the Moon on July 20, 1969, at 20:18 UTC. Armstrong became the first to step onto the lunar surface 6 hours later on July 21 at 02:56 UTC.</p>


Do we actually need all the wrapping text? It's a lot of it. Try to reduce it.

mlewand · 2018-01-31T14:36:14Z

tests/plugins/easyimage/manual/customtoolbar.md

@@ -0,0 +1,13 @@
+@bender-tags: 4.9.0, feature, 932
+@bender-ui: collapsed
+@bender-ckeditor-plugins: sourcearea, wysiwygarea, floatingspace, toolbar, easyimage


No need for sourcearea plugin here.

mlewand · 2018-01-31T16:01:18Z

plugins/easyimage/styles/easyimage.css

@@ -43,6 +46,15 @@ The outline is not a part of the element's dimensions, we have to use a border a
 	max-width: 25%;


With that style applied to alignCenter large images will be reduced to 25% of width.

Actually I find this entire block now very, very little readable. I think it would be better idea to split it into 3 groups (left, right, center). Even if it would cost us repeating max-width / float I feel that would be more readable.

I've limit center styles to 90% (just to differentiate them from full styles).

Sounds like a good idea 👍

mlewand · 2018-01-31T16:07:45Z

plugins/easyimage/plugin.js

+	 * style would produce `EasyimageLeft` button and `easyimageLeft` command.
+	 *
+	 * @since 4.9.0
+	 * @cfg {Object<string,Object>} easyimage_styles


We use uppercased String in docs.

The proper format would be Object.<foo, bar> (note the dot and space.

mlewand · 2018-01-31T16:11:46Z

plugins/easyimage/plugin.js

+	 * * `icon` - path to the icon used in the balloon toolbar,
+	 * * `iconHiDpi` - path to the high DPI version of the icon.
+	 *
+	 *		config.easyimage_styles = {


Code listing is not displayed properly:

Reported as ckeditor/jsduck#8. I've mitigated it by moving note "Every style added […]" directly under the list.

mlewand · 2018-01-31T16:21:33Z

tests/plugins/easyimage/manual/customstyles.html

+					attributes: {
+						'class': 'test'
+					},
+					icon: 'test'


Please add some title to it. Now it is undefined, which naturally lights a red bulb in tester mind:

mlewand · 2018-01-31T16:34:13Z

plugins/easyimage/plugin.js

+	 * items to the context menu for Easy Image widget.
+	 *
+	 * @since 4.9.0
+	 * @cfg {Array/String} [easyimage_toolbar=[ 'EasyimageFull', 'EasyimageSide', 'EasyimageAlt' ]]


It's expected to contain an array of strings, so accurate type hint is String[]/String.

mlewand · 2018-01-31T16:42:42Z

Also I have rebased this branch onto latest Easy Image feature branch in t/932-3393_rebased. Feel free to reuse it for your work, so that you don't have to resolve the conflicts by yourself.

Comandeer · 2018-02-01T09:31:42Z

File names for icons got long and tricky, there's easyimagealigncenter.png, easyimagealignleft.png and easyimagealignright.png. Since we already have easyimageside.png, how about making it a easyimagecenter.png (skip "align" part)? Simpler and more consistent.

Aren't icons names tied with button names? In that case it won't work unless integration with custom button icons is merged.

Comandeer · 2018-02-01T11:13:28Z

I've also rebased branch to newest t/932-2961.

mlewand

Generally the solution looks to be good, but requires some polishing/simplifications before going live.

Please add a rogue case, where one adds a style like:

{
	element: 'div',
	attributes: {
		'class': 'test'
	},
	label: 'Hacky style'
}

There should be assertion that element tag name has not changed. That will keep us safe form future regression assuming that widget<->style integration would have implemented handling for such styles.

mlewand · 2018-02-02T09:47:34Z

plugins/easyimage/plugin.js

+	 * @cfg {String} [easyimage_alignLeftClass='easyimage-alignLeft']
+	 * @member CKEDITOR.config
+	 */
+	CKEDITOR.config.easyimage_alignLeftClass = 'easyimage-alignLeft';


There's not much benefit from exposing easyimage_*Class members. Let's avoid redundancy, the proper way to configure that should be config.easyimage_styles option. We should reflect it in example listing of config.easyimage_styles.

Say you want to customize the class name for left align, then you should use:

config.easyimage_styles = { alignLeft: { attributes: { 'class': 'my-custom-left-align' } }, };

Note how I skipped label, button etc. It should be merged, thus the default should apply.

My brief testing shows that it already works that way, so that's cool.

This is exactly what you have covered in "test override default styles" test, and that's the way how it should be done.

I've also dropped easyimage_full and easyimage_side classes, the only one left is the main class for the widget.

Great! That's exactly what I've been looking for.

mlewand · 2018-02-02T09:47:52Z

plugins/easyimage/styles/easyimage.css

@@ -43,6 +46,15 @@ The outline is not a part of the element's dimensions, we have to use a border a
 	max-width: 25%;


Sounds like a good idea 👍

mlewand · 2018-02-02T10:12:26Z

plugins/easyimage/plugin.js


 		editor._.easyImageToolbarContext = editor.balloonToolbars.create( {
-			buttons: 'EasyimageFull,EasyimageSide,EasyimageAlt',
+			buttons: buttons.join ? buttons.join( ',' ) : buttons,
 			widgets: [ WIDGET_NAME ]
 		} );
 	}

 	function addMenuItems( editor ) {


Actually could we rename this function to something that mentions context menu? Because this is solely context menu integration thing.

mlewand · 2018-02-02T10:15:30Z

plugins/easyimage/plugin.js

@@ -10,12 +10,45 @@
 		WIDGET_NAME = 'easyimage';

 	function addCommands( editor ) {


This function now has too much logic. Instead it would be a good idea for us to split it into two functions with a clear task/responsibility - addCommands and addButtons.

I guess that would require you to make defaultStyles and editor.config.easyimage_styles union in parent closure, and pass the merged object as a parameter to both of them.

mlewand · 2018-02-02T10:19:14Z

plugins/easyimage/plugin.js

+				iconHiDpi: style.iconHiDpi
+			};
+
+			delete style.label;


Is this really needed? Will CKEDITOR.style break if it gets definition with stray properties?

If not this whole function is redundant.

mlewand · 2018-02-02T10:31:36Z

plugins/easyimage/plugin.js

@@ -30,105 +63,170 @@
 			};
 		}

-		function createCommand( exec, refreshCheck, forceSelectionCheck ) {
+		function createButton( button ) {
+			editor.ui.addButton( button.name, {


editor.ui.addButton could be unavailable if something changes in the dependency tree. Just to be safe I'd prefer adding that to requires list for this plugin.

button is already an explicit requirement for easyimage.

mlewand · 2018-02-02T11:11:57Z

plugins/easyimage/styles/easyimage.css

+	max-width: 90%;
+}
+
+.cke_widget_wrapper_easyimage-alignRight, :not(.cke_widget_wrapper_easyimage):not(.cke_widget_wrapper_easyimage-alignRight) > .easyimage-alignRight {


I think this particular selector could be joined to the .cke_widget_wrapper_easyimage-side, :not(.cke_widget_wrapper_easyimage):not(.cke_widget_wrapper_easyimage-side) > .easyimage-side selector above.

mlewand · 2018-02-02T11:13:11Z

plugins/easyimage/plugin.js

+	 *				},
+	 *				label: 'Align left',
+	 *				icon: '/foo/bar/icons/baz.png',
+	 *				iconHidpi: '/foo/bar/icons/hidpi/baz.png'


It's iconHiDpi (note letter casing).

mlewand · 2018-02-02T11:18:10Z

plugins/easyimage/plugin.js

+	/**
+	 * List of buttons to be displayed in a balloon toolbar for Easy Image widget.
+	 * If Context Menu plugin is enabled, this config variable will be used also to add
+	 * items to the context menu for Easy Image widget.


Please add a code listing where it uses left / center / right align buttons.

mlewand · 2018-02-02T11:29:35Z

plugins/easyimage/plugin.js

-		} );
+			styleDefinition.type = 'widget';
+			styleDefinition.widget = 'easyimage';
+			styleDefinition.group = 'easyimage';


What if we actually allowed to specify group? That would allow people to do these crazy border kind of things, without affecting the alignment.

So my thinking here is to make the group default to 'easyimage' if not defined, but preserve styleDefinition.group if present.

That would be nice option for extensibility.

With our current way of handling styles (buttons that allow only applying styles) such change will result in non-reversible styles. I'd leave it the way it is for now.

mlewand · 2018-02-02T11:36:57Z

Aren't icons names tied with button names? In that case it won't work unless integration with custom button icons is merged.

Make sense, let's leave it as it is now.

Comandeer · 2018-02-02T14:09:04Z

I've also rebased branch onto the latest t/932-2961.

mlewand

Good work with the tests.

I was missing a possibility to customize group for added styles. Played a bit and it works fine, which is a nice addition.

Also I was thinking if we could avoid data duplication (I'm thinking about _styles member and style name being kept in widget.data). I gave it a try and it was possible to reuse CKEDITOR.styleCommand which results with a single source of truth - reducing a risk of future mistakes by desyncing the two.

These were the biggest issues, other than that there were few minor things:

Tests not being ignored on unsupported environments.
No information in config docs about styles available by defalt.

Other than that code organization seemed to be fine.

I proposed changes in #1583.

That will help us smell some integration problems on other browsers if something goes wrong.

…OR.styleCommand.

The only source of information about applied style is the widget element itself. Also widget is no longer defaulted to a full style. Thus few TCs are failing in tests/plugins/easyimage/uploadintegrations suite.

It is now configurable with the confg.easyimage_defaultStyle option.

…o that only one listener is added per editor for a better performance.

…if widget already has one.

… button twice.

…. Minor simplification for refresh commands.

…API docs.

mlewand

With recent changes all looks good.

Styles system for Easy Image

f1ames mentioned this pull request Jan 31, 2018

Integrate button icons with easyimage styles #1555

Merged

2 tasks

mlewand suggested changes Jan 31, 2018

View reviewed changes

Comandeer force-pushed the t/932-3393 branch from 99857d4 to 2145411 Compare February 1, 2018 11:12

Comandeer requested a review from mlewand February 1, 2018 11:13

mlewand suggested changes Feb 2, 2018

View reviewed changes

Comandeer force-pushed the t/932-3393 branch from 2145411 to c6b5a76 Compare February 2, 2018 14:08

mlewand mentioned this pull request Feb 5, 2018

Corrections to Easy Image styles system #1583

Merged

2 tasks

mlewand self-requested a review February 5, 2018 09:37

mlewand suggested changes Feb 5, 2018

View reviewed changes

mlewand self-requested a review February 5, 2018 13:26

Comandeer added 15 commits February 5, 2018 14:26

Add easyimage_styles config variable.

51afb4e

Add easyimage_toolbar config variable.

6ea030a

Refactor internal addCommands function.

90a0920

Add manual tests.

2e4850e

Add basic support for custom styles.

d97e7df

Extract common tests tools to separate file.

4835c98

Integrate styles with ACF.

0cdc9ba

Simplify style commands naming rules.

50746de

Introduce default styles concept.

5e1a00f

Add language config.

bf6f50c

Add icons for align styles.

6df99ab

Add manual test.

98cd5ee

Add applying style when data is set, but the style is not applied.

8bb32f4

Update tests.

2dfdd86

Add test for overriding default style.

424f9b3

Comandeer and others added 25 commits February 5, 2018 14:26

Update manual test description.

c7762b9

Update API docs about style definitions.

23e394a

Tests: clarified TC name.

46707d6

Tests: restored tests for Easy Image alignment classes.

ca331b0

Tests: make sure that IE specific code is executed only for IE.

0f89f49

That will help us smell some integration problems on other browsers if something goes wrong.

Refactoring: a PoC Easy Image style implementation that reuses CKEDIT…

a23b6b2

…OR.styleCommand.

Refactoring: Easy Image widgets no longer store data.style property.

d56b1d4

The only source of information about applied style is the widget element itself. Also widget is no longer defaulted to a full style. Thus few TCs are failing in tests/plugins/easyimage/uploadintegrations suite.

Easy Image now again defaults to a certain style.

246dd98

It is now configurable with the confg.easyimage_defaultStyle option.

Tests: ignored unsupported platforms for Easy Image style tests.

d9528bb

Refactoring: simplified Easy Image refresh methods.

cd90595

Refactoring: simplified button-related logic in Easy Image.

13397b9

Tests: added manual test for Easy Image styles grouping.

19cac23

Refactoring: extracted afterCommandExec/beforeCommandExec listeners s…

06928c2

…o that only one listener is added per editor for a better performance.

Fixed a bug where default Easy Image style would be appended despite …

ed9c95d

…if widget already has one.

Tests: use centered images by default.

532d12b

Fixed Easy Image issue where the toolbar would hide if you press same…

6054038

… button twice.

Changed Easy Image default classes names to dash-separated convention…

677da86

…. Minor simplification for refresh commands.

Docs: API docs enhancements in Easy Image styles.

d9c8c93

Tests: corrected default classes in the manual test.

ecf4aab

Corrected Easy Image align selectors in CSS stylesheets.

a7d14bf

Tests: added missing quotes to manual steps description.

94b5134

Fix typos in API docs.

1ac1d18

Remove redundant assertions from unit tests.

dbaee7a

Tests: corrected an incomplete sentence.

3af8669

Docs: added more complex code listing to the config.easyimage_styles …

377f6e4

…API docs.

mlewand force-pushed the t/932-3393 branch from 721bd11 to 377f6e4 Compare February 5, 2018 13:28

mlewand approved these changes Feb 5, 2018

View reviewed changes

mlewand merged commit 686d0eb into t/932-2961 Feb 5, 2018

mlewand added a commit that referenced this pull request Feb 5, 2018

Merge pull request #1518 from ckeditor/t/932-3393

611e7e4

Styles system for Easy Image

Comandeer pushed a commit that referenced this pull request Feb 6, 2018

Merge pull request #1518 from ckeditor/t/932-3393

3976488

Styles system for Easy Image

		@@ -43,6 +46,15 @@ The outline is not a part of the element's dimensions, we have to use a border a
		max-width: 25%;

		@@ -10,12 +10,45 @@
		WIDGET_NAME = 'easyimage';

		function addCommands( editor ) {

Styles system for Easy Image #1518

Styles system for Easy Image #1518

Conversation

Comandeer commented Jan 29, 2018

What is the purpose of this pull request?

Does your PR contain necessary tests?

This PR contains

What changes did you make?

mlewand 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

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

mlewand commented Jan 31, 2018

Comandeer commented Feb 1, 2018

Comandeer commented Feb 1, 2018

mlewand 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

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

mlewand commented Feb 2, 2018

Comandeer commented Feb 2, 2018 • edited Loading

mlewand left a comment

Choose a reason for hiding this comment

mlewand left a comment

Choose a reason for hiding this comment

Comandeer commented Feb 2, 2018 •

edited

Loading