-
Notifications
You must be signed in to change notification settings - Fork 37
ImageStyle #23
Changes from 25 commits
74ec5d3
777827a
af6541d
9b7a556
26578db
0f96c7b
8069e19
b8a6178
98e4051
a005ff7
6e15b81
7004581
18b2819
8267f6f
b90621f
bb60e25
7ec5d65
21c86c7
12fbc44
3f09d99
cd305e3
7a2b3a0
c6948e5
8493080
091a128
1f511d5
d953a7b
40f1907
7327f57
12d75d9
c350b10
9ba4a3d
cc7370f
a174bdc
0b39383
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,86 @@ | ||
/** | ||
* @license Copyright (c) 2003-2016, CKSource - Frederico Knabben. All rights reserved. | ||
* For licensing, see LICENSE.md. | ||
*/ | ||
|
||
/** | ||
* @module image/imagestyle/converters | ||
*/ | ||
|
||
import { isImage, getStyleByValue } 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 ) { | ||
return; | ||
} | ||
|
||
viewElement.removeClass( oldStyle.className ); | ||
} | ||
|
||
if ( eventType == 'addAttribute' || eventType == 'changeAttribute' ) { | ||
if ( !newStyle ) { | ||
return; | ||
} | ||
|
||
viewElement.addClass( newStyle.className ); | ||
} | ||
|
||
consumable.consume( data.item, consumableType ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is it kept here? Can't it replace the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ); | ||
}; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,76 @@ | ||
/** | ||
* @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 ); | ||
} | ||
|
||
// If there is no default image toolbar configuration, add all image styles buttons. | ||
const imageToolbarConfig = this.editor.config.get( 'image.toolbar' ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Sounds reasonable? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right. I didn't take other plugins into consideration. So we will use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. |
||
|
||
if ( !imageToolbarConfig ) { | ||
this.editor.config.set( 'image.toolbar', styles.map( style => 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( 'imagestyle' ); | ||
const t = editor.t; | ||
|
||
editor.ui.componentFactory.add( style.name, ( locale ) => { | ||
const view = new ButtonView( locale ); | ||
|
||
view.set( { | ||
label: t( style.title ), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It can't be used like that. |
||
icon: style.icon | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can now also add |
||
} ); | ||
|
||
view.bind( 'isEnabled' ).to( command, 'isEnabled' ); | ||
view.bind( 'isOn' ).to( command, 'value', ( commandValue ) => { | ||
return commandValue == style.value; | ||
} ); | ||
|
||
this.listenTo( view, 'execute', () => editor.execute( 'imagestyle', { value: style.value } ) ); | ||
|
||
return view; | ||
} ); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,118 @@ | ||
/** | ||
* @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, getStyleByValue } 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 command. | ||
* | ||
* @param {module:core/editor/editor~Editor} editor Editor instance. | ||
* @param {Array.<module:image/imagestyle/imagestyleengine~ImageStyleFormat>} styles Allowed styles. | ||
*/ | ||
constructor( editor, styles ) { | ||
super( editor ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing new line. |
||
/** | ||
* The current style value. | ||
* | ||
* @readonly | ||
* @observable | ||
* @member {String} #value | ||
*/ | ||
this.set( 'value', false ); | ||
|
||
/** | ||
* Allowed image styles used by this command. | ||
* | ||
* @readonly | ||
* @member {Array.<module:image/imagestyle/imagestyleengine~ImageStyleFormat>} #styles | ||
*/ | ||
this.styles = styles; | ||
|
||
// 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 ( isImage( element ) ) { | ||
if ( element.hasAttribute( 'imageStyle' ) ) { | ||
const value = element.getAttribute( 'imageStyle' ); | ||
|
||
// Check if value exists. | ||
this.value = ( getStyleByValue( value, this.styles ) ? value : false ); | ||
} else { | ||
// When there is no `style` attribute - set value to null. | ||
this.value = null; | ||
} | ||
} else { | ||
this.value = false; | ||
} | ||
} | ||
|
||
/** | ||
* @inheritDoc | ||
*/ | ||
_checkEnabled() { | ||
const element = this.editor.document.selection.getSelectedElement(); | ||
|
||
return isImage( element ); | ||
} | ||
|
||
/** | ||
* Executes command. | ||
* | ||
* @protected | ||
* @param {Object} options | ||
* @param {String} options.value Value to apply. It must be one of the values from styles passed to {@link #constructor}. | ||
* @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 ) { | ||
const currentValue = this.value; | ||
const newValue = options.value; | ||
|
||
// Check if new value is valid. | ||
if ( !getStyleByValue( newValue, this.styles ) ) { | ||
return; | ||
} | ||
|
||
// Stop if same value is already applied. | ||
if ( currentValue == newValue ) { | ||
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', newValue ); | ||
} ); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,90 @@ | ||
/** | ||
* @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 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: 'Full size image', icon: fullSizeIcon, value: null }, | ||
|
||
// This represents side image. | ||
{ name: 'imageStyleSide', title: '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 ); | ||
|
||
// Converter for figure element from view to model. | ||
for ( let style of styles ) { | ||
// Create converter only for non-null values. | ||
if ( style.value !== null ) { | ||
data.viewToModel.on( 'element:figure', viewToModelImageStyle( style ), { priority: 'low' } ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 image style command. | ||
editor.commands.set( 'imagestyle', new ImageStyleCommand( editor, styles ) ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
} | ||
} | ||
|
||
/** | ||
* Image style format descriptor. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Some example would do nicely :P |
||
* | ||
* @typedef {Object} module:image/imagestyle/imagestyleengine~ImageStyleFormat | ||
* @property {String} name Name of the style, it will be used to store style's button under that name 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. | ||
*/ |
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.
I don't get it... why do you check it here?
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.
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.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.
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 toimageStyle
attribute.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.
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:
(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.