Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

ImageStyle #23

Merged
merged 35 commits into from
Jan 10, 2017
Merged
Show file tree
Hide file tree
Changes from 29 commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
74ec5d3
Added ImageStyleCommand class.
szymonkups Dec 15, 2016
777827a
Added first implementation of converters for image styles.
szymonkups Dec 16, 2016
af6541d
Update image style command, added manual test.
szymonkups Dec 19, 2016
9b7a556
Added image toolbar.
szymonkups Dec 19, 2016
26578db
Added tests to ImageStyleCommand.
szymonkups Dec 21, 2016
0f96c7b
Updated docs in ImageStyleCommand.
szymonkups Dec 21, 2016
8069e19
Updated docs and tests for ImageStyleEngine.
szymonkups Dec 21, 2016
b8a6178
Added styles to ImageStyleEngine.
szymonkups Dec 22, 2016
98e4051
Simplified converters.
szymonkups Dec 22, 2016
a005ff7
Changed ImageStyle configuration object to array.
szymonkups Dec 22, 2016
6e15b81
Added docs and tests to ImageStyle plugin.
szymonkups Dec 22, 2016
7004581
Docs fixes.
szymonkups Dec 22, 2016
18b2819
Fixed model to view converter for image styles.
szymonkups Dec 22, 2016
8267f6f
Updated description of image style manual test.
szymonkups Dec 22, 2016
b90621f
Added tests for image styles utilities.
szymonkups Dec 22, 2016
bb60e25
One converter for model to view conversion in ImageStyleEngine.
szymonkups Dec 22, 2016
7ec5d65
Added lodash throttle method to scroll and resize events in ImageTool…
szymonkups Dec 28, 2016
21c86c7
ImageStyle adds all styles buttons to ImageToolbar if no other config…
szymonkups Dec 28, 2016
12fbc44
Added tests to ImageToolbar.
szymonkups Dec 29, 2016
3f09d99
Use utils globals in tests.
szymonkups Dec 29, 2016
cd305e3
Docs typo.
szymonkups Dec 29, 2016
7a2b3a0
Removed unused ClickObserver from ImageToolbar.
szymonkups Dec 30, 2016
c6948e5
Attaching same scroll and resize callback in ImageToolbar.
szymonkups Dec 30, 2016
8493080
Updated paths.
szymonkups Jan 2, 2017
091a128
Added proper icons to image styles.
szymonkups Jan 3, 2017
1f511d5
Minor fixes.
szymonkups Jan 4, 2017
d953a7b
Changed ImageToolbar default configuration processing.
szymonkups Jan 4, 2017
40f1907
Creating separate command for each style.
szymonkups Jan 4, 2017
7327f57
Refactoring util methods.
szymonkups Jan 4, 2017
12d75d9
ImageStyle - converting from view to model in one callback for all st…
szymonkups Jan 9, 2017
c350b10
Split model to view converter in image style.
szymonkups Jan 9, 2017
9ba4a3d
Using buildViewConverter for converting image styles.
szymonkups Jan 10, 2017
cc7370f
Revert "Using buildViewConverter for converting image styles."
szymonkups Jan 10, 2017
a174bdc
Code refactoring: simplified logic.
Reinmar Jan 10, 2017
0b39383
Code refactoring: renamed functions.
Reinmar Jan 10, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
98 changes: 98 additions & 0 deletions src/imagestyle/converters.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
/**
* @license Copyright (c) 2003-2016, CKSource - Frederico Knabben. All rights reserved.
* For licensing, see LICENSE.md.
*/

/**
* @module image/imagestyle/converters
*/

import { isImage } from '../utils';

/**
* Returns converter for `imageStyle` attribute. It can be used for adding, changing and removing the attribute.
*
* @param {Object} styles Object containing available styles. See {@link module:image/imagestyle/imagestyleengine~ImageStyleFormat}
* for more details.
* @returns {Function} Model to view attribute converter.
*/
export function modelToViewSetStyle( styles ) {
return ( evt, data, consumable, conversionApi ) => {
const eventType = evt.name.split( ':' )[ 0 ];
const consumableType = eventType + ':imageStyle';

if ( !consumable.test( data.item, consumableType ) ) {
return;
}

// Check if there is class name associated with given value.
const newStyle = getStyleByValue( data.attributeNewValue, styles );
const oldStyle = getStyleByValue( data.attributeOldValue, styles );
const viewElement = conversionApi.mapper.toViewElement( data.item );

if ( eventType == 'changeAttribute' || eventType == 'removeAttribute' ) {
if ( !oldStyle ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@szymonkups szymonkups Jan 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@Reinmar Reinmar Jan 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return;
}

viewElement.removeClass( oldStyle.className );
}

if ( eventType == 'addAttribute' || eventType == 'changeAttribute' ) {
if ( !newStyle ) {
return;
}

viewElement.addClass( newStyle.className );
}

consumable.consume( data.item, consumableType );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

};
}

/**
* Returns view to model converter converting image style CSS class to proper value in the model.
*
* @param {module:image/imagestyle/imagestyleengine~ImageStyleFormat} style Style for which converter is created.
* @returns {Function} View to model converter.
*/
export function viewToModelImageStyle( style ) {
return ( evt, data, consumable, conversionApi ) => {
const viewFigureElement = data.input;
const modelImageElement = data.output;

// *** Step 1: Validate conversion.
// Check if view element has proper class to consume.
if ( !consumable.test( viewFigureElement, { class: style.className } ) ) {
return;
}

// Check if figure is converted to image.
if ( !isImage( modelImageElement ) ) {
return;
}

// Check if image element can be placed in current context wit additional attribute.
const attributes = [ ...modelImageElement.getAttributeKeys(), 'imageStyle' ];

if ( !conversionApi.schema.check( { name: 'image', inside: data.context, attributes } ) ) {
return;
}

// *** Step2: Convert to model.
consumable.consume( viewFigureElement, { class: style.className } );
modelImageElement.setAttribute( 'imageStyle', style.value );
};
}

// Returns style with given `value` from array of styles.
// @param {String} value
// @param {Array.<module:image/imagestyle/imagestyleengine~ImageStyleFormat> } styles
// @return {module:image/imagestyle/imagestyleengine~ImageStyleFormat|undefined}
function getStyleByValue( value, styles ) {
for ( let style of styles ) {
if ( style.value === value ) {
return style;
}
}
}
74 changes: 74 additions & 0 deletions src/imagestyle/imagestyle.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
/**
* @license Copyright (c) 2003-2016, CKSource - Frederico Knabben. All rights reserved.
* For licensing, see LICENSE.md.
*/

/**
* @module image/imagestyle/imagestyle
*/

import Plugin from 'ckeditor5-core/src/plugin';
import ImageStyleEngine from './imagestyleengine';
import ButtonView from 'ckeditor5-ui/src/button/buttonview';

/**
* The image style plugin.
*
* Uses {@link module:image/imagestyle/imagestyleengine~ImageStyleEngine}.
*
* @extends module:core/plugin~Plugin
*/
export default class ImageStyle extends Plugin {
/**
* @inheritDoc
*/
static get requires() {
return [ ImageStyleEngine ];
}

/**
* @inheritDoc
*/
init() {
const styles = this.editor.config.get( 'image.styles' );

for ( let style of styles ) {
this._createButton( style );
}

// Push buttons to default image toolbar if one exists.
const defaultImageToolbarConfig = this.editor.config.get( 'image.defaultToolbar' );

if ( defaultImageToolbarConfig ) {
styles.forEach( style => defaultImageToolbarConfig.push( style.name ) );
}
}

/**
* Creates button for each style and stores it in editor's {@link module:ui/componentfactory~ComponentFactory ComponentFactory}.
*
* @private
* @param {module:image/imagestyle/imagestyleengine~ImageStyleFormat} style
*/
_createButton( style ) {
const editor = this.editor;
const command = editor.commands.get( style.name );

editor.ui.componentFactory.add( style.name, ( locale ) => {
const view = new ButtonView( locale );

view.set( {
label: style.title,
icon: style.icon,
tooltip: true
} );

view.bind( 'isEnabled' ).to( command, 'isEnabled' );
view.bind( 'isOn' ).to( command, 'value' );

this.listenTo( view, 'execute', () => editor.execute( style.name ) );

return view;
} );
}
}
109 changes: 109 additions & 0 deletions src/imagestyle/imagestylecommand.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
/**
* @license Copyright (c) 2003-2016, CKSource - Frederico Knabben. All rights reserved.
* For licensing, see LICENSE.md.
*/

/**
* @module image/imagestyle/imagestylecommand
*/

import Command from 'ckeditor5-core/src/command/command';
import { isImage } from '../utils';

/**
* The image style command. It is used to apply different image styles.
*
* @extends module:core/command/command~Command
*/
export default class ImageStyleCommand extends Command {
/**
* Creates instance of the image style command. Each command instance is handling one style.
*
* @param {module:core/editor/editor~Editor} editor Editor instance.
* @param {module:image/imagestyle/imagestyleengine~ImageStyleFormat} styles Style to apply by this command.
*/
constructor( editor, style ) {
super( editor );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing new line.


/**
* The current command value - `true` if style handled by the command is applied on currently selected image,
* `false` otherwise.
*
* @readonly
* @observable
* @member {Boolean} #value
*/
this.set( 'value', false );

/**
* Style handled by this command.
*
* @readonly
* @member {module:image/imagestyle/imagestyleengine~ImageStyleFormat} #style
*/
this.style = style;

// Update current value and refresh state each time something change in model document.
this.listenTo( editor.document, 'changesDone', () => {
this._updateValue();
this.refreshState();
} );
}

/**
* Updates command's value.
*
* @private
*/
_updateValue() {
const doc = this.editor.document;
const element = doc.selection.getSelectedElement();

if ( !element ) {
this.value = false;

return;
}

if ( this.style.value === null ) {
this.value = !element.hasAttribute( 'imageStyle' );
} else {
this.value = ( element.getAttribute( 'imageStyle' ) == this.style.value );
}
}

/**
* @inheritDoc
*/
_checkEnabled() {
const element = this.editor.document.selection.getSelectedElement();

return isImage( element );
}

/**
* Executes command.
*
* @protected
* @param {Object} options
* @param {module:engine/model/batch~Batch} [options.batch] Batch to collect all the change steps. New batch will be
* created if this option is not set.
*/
_doExecute( options = {} ) {
// Stop if style is already applied.
if ( this.value ) {
return;
}

const editor = this.editor;
const doc = editor.document;
const selection = doc.selection;
const imageElement = selection.getSelectedElement();

doc.enqueueChanges( () => {
const batch = options.batch || doc.batch();

batch.setAttribute( imageElement, 'imageStyle', this.style.value );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

} );
}
}
102 changes: 102 additions & 0 deletions src/imagestyle/imagestyleengine.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
/**
* @license Copyright (c) 2003-2016, CKSource - Frederico Knabben. All rights reserved.
* For licensing, see LICENSE.md.
*/

/**
* @module image/imagestyle/imagestyleengine
*/

import Plugin from 'ckeditor5-core/src/plugin';
import ImageStyleCommand from './imagestylecommand';
import ImageEngine from '../imageengine';
import { viewToModelImageStyle, modelToViewSetStyle } from './converters';
import fullSizeIcon from 'ckeditor5-core/theme/icons/object-center.svg';
import sideIcon from 'ckeditor5-core/theme/icons/object-right.svg';

/**
* The image style engine plugin. Sets default configuration, creates converters and registers
* {@link module:image/imagestyle/imagestylecommand~ImageStyleCommand ImageStyleCommand}.
*
* @extends {module:core/plugin~Plugin}
*/
export default class ImageStyleEngine extends Plugin {
/**
* @inheritDoc
*/
static get requires() {
return [ ImageEngine ];
}

/**
* @inheritDoc
*/
init() {
const editor = this.editor;
const t = editor.t;
const doc = editor.document;
const schema = doc.schema;
const data = editor.data;
const editing = editor.editing;

// Define default configuration.
editor.config.define( 'image.styles', [
// This option is equal to situation when no style is applied.
{ name: 'imageStyleFull', title: t( 'Full size image' ), icon: fullSizeIcon, value: null },

// This represents side image.
{ name: 'imageStyleSide', title: t( 'Side image' ), icon: sideIcon, value: 'side', className: 'image-style-side' }
] );

// Get configuration.
const styles = editor.config.get( 'image.styles' );

// Allow imageStyle attribute in image.
// We could call it 'style' but https://github.com/ckeditor/ckeditor5-engine/issues/559.
schema.allow( { name: 'image', attributes: 'imageStyle', inside: '$root' } );

// Converters for imageStyle attribute from model to view.
const modelToViewConverter = modelToViewSetStyle( styles );
editing.modelToView.on( 'addAttribute:imageStyle:image', modelToViewConverter );
data.modelToView.on( 'addAttribute:imageStyle:image', modelToViewConverter );
editing.modelToView.on( 'changeAttribute:imageStyle:image', modelToViewConverter );
data.modelToView.on( 'changeAttribute:imageStyle:image', modelToViewConverter );
editing.modelToView.on( 'removeAttribute:imageStyle:image', modelToViewConverter );
data.modelToView.on( 'removeAttribute:imageStyle:image', modelToViewConverter );

for ( let style of styles ) {
// 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' } );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

// Register separate command for each style.
editor.commands.set( style.name, new ImageStyleCommand( editor, style ) );
}
}
}

/**
* Image style format descriptor.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some example would do nicely :P

*
* import fullIcon from 'path/to/icon.svg`;
*
* const imageStyleFormat = {
* name: 'fullSizeImage',
* value: 'full',
* icon: fullIcon,
* title: `Full size image`,
* class: `image-full-size`
* }
*
* @typedef {Object} module:image/imagestyle/imagestyleengine~ImageStyleFormat
* @property {String} name Name of the style. It will be used to:
* * register {@link module:core/command/command~Command command} which will apply this style,
* * store style's button in editor's {@link module:ui/componentfactory~ComponentFactory ComponentFactory}.
* @property {String} value Value used to store this style in model attribute.
* When value is `null` style will be used as default one. Default style does not apply any CSS class to the view element.
* @property {String} icon SVG icon representation to use when creating style's button.
* @property {String} title Style's title.
* @property {String} className CSS class used to represent style in view.
*/
Loading