ImageStyle #23

szymonkups · 2016-12-29T09:12:22Z

Fixes ckeditor/ckeditor5#5040.
~~For now icons are added inside this PR, this will change when common icons will land in core: https://github.com/ckeditor/ckeditor5-theme-lark/issues/59.~~

EDIT:
~~For now core icons align-center.svg and align-right.svg are used. This will be changed to when proper icons land in core: https://github.com/ckeditor/ckeditor5-theme-lark/issues/59.~~

EDIT v2: Proper icons are used.

…bar, docs.

…uration is provided.

Reinmar · 2017-01-04T09:57:54Z

src/imagestyle/imagestyle.js

+		}
+
+		// If there is no default image toolbar configuration, add all image styles buttons.
+		const imageToolbarConfig = this.editor.config.get( 'image.toolbar' );


This is not going to work this way... Because, if there's any other plugin which would like to extend this toolbar, it won't do that, because the toolbar is already set by this plugin.

So, unfortunately, we must make this a bit more complex.

The assumption is that ImageToolbar is loaded and sets image.defaultToolbar to an empty array in its constructor (which is "before init"). If it's not loaded, the property doesn't exist.

If the property exists, the other plugins should then push their default buttons to this array in their init() callbacks.

The ImageToolbar should then use image.toolbar or image.defaultToolbar.

Sounds reasonable?

You're right. I didn't take other plugins into consideration. So we will use image.defaultToolbar configuration only if image.toolbar is not specified. Is that correct?

Reinmar

The feature works great and there are just 2 rather simple code refactorings to do – the command split and how the default config is created.

Reinmar · 2017-01-04T10:00:24Z

src/imagestyle/imagestylecommand.js

+	 * @param {Array.<module:image/imagestyle/imagestyleengine~ImageStyleFormat>} styles Allowed styles.
+	 */
+	constructor( editor, styles ) {
+		super( editor );


Missing new line.

Reinmar · 2017-01-04T10:05:09Z

src/imagestyle/imagestyle.js

+			const view = new ButtonView( locale );
+
+			view.set( {
+				label: t( style.title ),


It can't be used like that. t() must be always used with a string. So the t() function should be used here: https://github.com/ckeditor/ckeditor5-image/pull/23/files#diff-00366adf927974c568c9ea53d58ede26R44

Reinmar · 2017-01-04T10:05:29Z

src/imagestyle/imagestyle.js

+
+			view.set( {
+				label: t( style.title ),
+				icon: style.icon


You can now also add tooltip: true

Reinmar · 2017-01-04T10:10:00Z

src/imagestyle/imagestyleengine.js

+		}
+
+		// Register image style command.
+		editor.commands.set( 'imagestyle', new ImageStyleCommand( editor, styles ) );


We're rather going to split the heading command into multiple heading commands, so the same should happen here. See https://github.com/ckeditor/ckeditor5-heading/issues/53. There should be a separate command per each style.

Reinmar · 2017-01-04T10:10:24Z

src/imagestyle/imagestyleengine.js

+}
+
+/**
+ * Image style format descriptor.


Some example would do nicely :P

Reinmar · 2017-01-04T10:11:26Z

src/imagestyle/utils.js

+ * @param {module:engine/model/element~Element} modelElement
+ * @returns {Boolean}
+ */
+export function isImage( modelElement ) {


Would rather make for some base image utils. I can see there src/utils.js.

Reinmar · 2017-01-04T10:12:30Z

src/imagestyle/utils.js

+ * @param {Array.<module:image/imagestyle/imagestyleengine~ImageStyleFormat> } styles
+ * @return {module:image/imagestyle/imagestyleengine~ImageStyleFormat|undefined}
+ */
+export function getStyleByValue( value, styles ) {


I guess this won't be necessary after the proposed change to split the command.

It is also used by converters to be sure if applied value is correct with provided configuration.

But now it is used only there and looks like it can be just a private to that module. This way we can get rid of utils file.

Reinmar · 2017-01-04T10:13:26Z

src/imagetoolbar.js

+		Template.extend( panel.template, {
+			attributes: {
+				class: [
+					'ck-toolbar__container',


Trailing comma.

Reinmar · 2017-01-09T11:18:19Z

src/imagestyle/converters.js

+			viewElement.addClass( newStyle.className );
+		}
+
+		consumable.consume( data.item, consumableType );


Why is it kept here? Can't it replace the consumable.test() call?

I did some other checks and return points in this function so I wanted to test first and then consume when everything is validated.

Reinmar · 2017-01-09T11:18:58Z

src/imagestyle/converters.js

+		const viewElement = conversionApi.mapper.toViewElement( data.item );
+
+		if ( eventType == 'changeAttribute' || eventType == 'removeAttribute' ) {
+			if ( !oldStyle ) {


I don't get it... why do you check it here?

The style attribute is used by this feature. If the getStyleByValue method can't find it, then something's really broken and we're just hiding the bug this way.

I wasn't sure how to handle it to be honest. On the one hand we have ImageStyle plugin that have its configuration and I wanted to build a converter that will only handle values from that configuration. On the other hand - there won't be probably any other plugin that will write something to imageStyle attribute.

Actually, that condition could even be there, but if it wasn't so... distinguished :D. The code needs to be intuitive. Now, you read "if something, then ... if not something ABORT... do something". This isn't very natural of expressing this logic.

This would be better:

if ( oldStyle && needToRemove( eventName ) ) { remove(); }

(See how you don't have to parse eventName in this callback and how well the code reads?)

And I agree – it's more error safe to check if oldStyle was found. But how was the value of the model element attribute set in the first place? There would need to be an additional feature which uses the same attribute – I don't think this should happen. But I may be wrong :P.

Anyway, since you have tests for this already, then we can keep it. Just make the code more intuitive. Also, you could consider splitting the function to two – one to handle change and remove attribute and other for add and change.

I also wrote a comment in https://github.com/ckeditor/ckeditor5-engine/issues/736 that we need better tools to write converters, because, e.g. in this case, there are too many listeners needed. Let's keep collecting use case to simplify.

Reinmar · 2017-01-09T11:28:29Z

src/imagestyle/imagestyleengine.js

+			// Converter for figure element from view to model.
+			// Create converter only for non-null values.
+			if ( style.value !== null ) {
+				data.viewToModel.on( 'element:figure', viewToModelImageStyle( style ), { priority: 'low' } );


Can't we have a single listener for all styles? Like for the opposite conversion?

That's a good point, I'll try to do this with one listener.

Reinmar · 2017-01-09T11:29:46Z

src/imagestyle/imagestylecommand.js

+		doc.enqueueChanges( () => {
+			const batch = options.batch || doc.batch();
+
+			batch.setAttribute( imageElement, 'imageStyle', this.style.value );


The attribute should be called 'style'. It's image element's attribute after all.

I wanted to call it 'style' but we have this: https://github.com/ckeditor/ckeditor5-engine/issues/559 that prevents us from using test utils.

That's a very bad reason for choosing this name :P. Let's see if we can do something to fix that bug. It's a very serious problem.

Note: This is quite important what attribute name we use because this is kind of a public API of this feature.

That's a very bad reason for choosing this name :P. Let's see if we can do something to fix that bug. It's a very serious problem.

I will fix other issues with this PR and then look into that issue.

I've created separate issue for that: https://github.com/ckeditor/ckeditor5-image/issues/25.

Reinmar

Some questions regarding conversion and one issue (name of the attribute).

…yles.

This reverts commit 9ba4a3d.

szymonkups requested a review from Reinmar December 29, 2016 09:12

szymonkups added 24 commits January 2, 2017 09:41

Added ImageStyleCommand class.

74ec5d3

Added first implementation of converters for image styles.

777827a

Update image style command, added manual test.

af6541d

Added image toolbar.

9b7a556

Added tests to ImageStyleCommand.

26578db

Updated docs in ImageStyleCommand.

0f96c7b

Updated docs and tests for ImageStyleEngine.

8069e19

Added styles to ImageStyleEngine.

b8a6178

Simplified converters.

98e4051

Changed ImageStyle configuration object to array.

a005ff7

Added docs and tests to ImageStyle plugin.

6e15b81

Docs fixes.

7004581

Fixed model to view converter for image styles.

18b2819

Updated description of image style manual test.

8267f6f

Added tests for image styles utilities.

b90621f

One converter for model to view conversion in ImageStyleEngine.

bb60e25

Added lodash throttle method to scroll and resize events in ImageTool…

7ec5d65

…bar, docs.

ImageStyle adds all styles buttons to ImageToolbar if no other config…

21c86c7

…uration is provided.

Added tests to ImageToolbar.

12fbc44

Use utils globals in tests.

3f09d99

Docs typo.

cd305e3

Removed unused ClickObserver from ImageToolbar.

7a2b3a0

Attaching same scroll and resize callback in ImageToolbar.

c6948e5

Updated paths.

8493080

szymonkups force-pushed the t/18 branch from 8243e09 to 8493080 Compare January 2, 2017 10:49

Added proper icons to image styles.

091a128

Reinmar reviewed Jan 4, 2017

View reviewed changes

Reinmar suggested changes Jan 4, 2017

View reviewed changes

Minor fixes.

1f511d5

szymonkups added 3 commits January 4, 2017 13:22

Changed ImageToolbar default configuration processing.

d953a7b

Creating separate command for each style.

40f1907

Refactoring util methods.

7327f57

Reinmar reviewed Jan 9, 2017

View reviewed changes

Reinmar suggested changes Jan 9, 2017

View reviewed changes

szymonkups and others added 6 commits January 9, 2017 17:46

ImageStyle - converting from view to model in one callback for all st…

12d75d9

…yles.

Split model to view converter in image style.

c350b10

Using buildViewConverter for converting image styles.

9ba4a3d

Revert "Using buildViewConverter for converting image styles."

cc7370f

This reverts commit 9ba4a3d.

Code refactoring: simplified logic.

a174bdc

Code refactoring: renamed functions.

0b39383

Reinmar merged commit 4b8e71b into master Jan 10, 2017

Reinmar deleted the t/18 branch January 10, 2017 15:43

oleq mentioned this pull request Mar 23, 2021

Managing multiple block images ckeditor/ckeditor5#9183

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ImageStyle #23

ImageStyle #23

szymonkups commented Dec 29, 2016 •

edited by pomek

Loading

Reinmar Jan 4, 2017 •

edited

Loading

szymonkups Jan 4, 2017

Reinmar Jan 4, 2017

Reinmar left a comment

Reinmar Jan 4, 2017

Reinmar Jan 4, 2017

Reinmar Jan 4, 2017

Reinmar Jan 4, 2017

Reinmar Jan 4, 2017

Reinmar Jan 4, 2017

Reinmar Jan 4, 2017

szymonkups Jan 4, 2017

szymonkups Jan 4, 2017

Reinmar Jan 4, 2017

Reinmar Jan 9, 2017

szymonkups Jan 9, 2017

Reinmar Jan 9, 2017

Reinmar Jan 9, 2017

szymonkups Jan 9, 2017 •

edited

Loading

Reinmar Jan 9, 2017 •

edited

Loading

Reinmar Jan 9, 2017

szymonkups Jan 9, 2017

Reinmar Jan 9, 2017

szymonkups Jan 9, 2017

Reinmar Jan 9, 2017

Reinmar Jan 9, 2017

szymonkups Jan 9, 2017

szymonkups Jan 10, 2017

Reinmar left a comment

ImageStyle #23

ImageStyle #23

Conversation

szymonkups commented Dec 29, 2016 • edited by pomek Loading

Reinmar Jan 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Reinmar 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

szymonkups Jan 9, 2017 • edited Loading

Choose a reason for hiding this comment

Reinmar Jan 9, 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

Reinmar left a comment

Choose a reason for hiding this comment

szymonkups commented Dec 29, 2016 •

edited by pomek

Loading

Reinmar Jan 4, 2017 •

edited

Loading

szymonkups Jan 9, 2017 •

edited

Loading

Reinmar Jan 9, 2017 •

edited

Loading