-
Notifications
You must be signed in to change notification settings - Fork 40
T/1198: ViewElementConfig with helper converters. #1205
Changes from 13 commits
2b4352a
ea95029
1e28cba
61ea426
b2f1044
a54727d
d7141cc
7381a09
291d889
1328204
f80abc2
2ac20cc
4604f04
cda98ba
16cae1d
67de247
7d30fa1
86be1e7
e2ce318
a48f922
f6b5676
b77b76f
42848f7
55f7555
5e4ebda
211d748
548734e
4760cce
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,310 @@ | ||
/** | ||
* @license Copyright (c) 2003-2017, CKSource - Frederico Knabben. All rights reserved. | ||
* For licensing, see LICENSE.md. | ||
*/ | ||
|
||
/** | ||
* @module engine/conversion/configurationdefinedconverters | ||
*/ | ||
|
||
import AttributeElement from '../view/attributeelement'; | ||
import ViewContainerElement from '../view/containerelement'; | ||
|
||
import buildModelConverter from './buildmodelconverter'; | ||
import buildViewConverter from './buildviewconverter'; | ||
|
||
/** | ||
* Helper for creating model to view converter from model's element | ||
* to {@link module:engine/view/containerelement~ContainerElement ViewContainerElement}. The `acceptAlso` property is ignored. | ||
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. What does the last sentence do here? You're introducing the function, so don't go into details like what kind of property is ignored because you haven't introduced this property yet. |
||
* | ||
* You can define conversion as simple model element to view element conversion using simplified definition: | ||
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 think it's always "by using". "using" means that the definition is an unrelated condition. https://english.stackexchange.com/questions/217815/what-is-difference-between-using-and-by-using The same applies below – "by defining". |
||
* | ||
* modelElementToViewContainerElement( { | ||
* model: 'heading1', | ||
* view: 'h1', | ||
* }, [ dispatcher ] ); | ||
* | ||
* Or defining full-flavored view object: | ||
* | ||
* modelElementToViewContainerElement( { | ||
* model: 'heading1', | ||
* view: { | ||
* name: 'h1', | ||
* class: [ 'header', 'article-header' ], | ||
* attributes: { | ||
* data-header: 'level-1', | ||
* } | ||
* }, | ||
* }, [ dispatcher ] ); | ||
* | ||
* Above will generate an HTML tag: | ||
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. "the following DOM element:" 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. actually, isn't it a view element? |
||
* | ||
* <h1 class="header article-header" data-header="level-1">...</h1> | ||
* | ||
* @param {module:engine/conversion/configurationdefinedconverters~ConverterDefinition} definition A conversion configuration. | ||
* @param {Array.<module:engine/conversion/modelconversiondispatcher~ModelConversionDispatcher>} dispatchers | ||
*/ | ||
export function modelElementToViewContainerElement( definition, dispatchers ) { | ||
const { model: modelElement, viewDefinition } = parseConverterDefinition( definition ); | ||
|
||
buildModelConverter() | ||
.for( ...dispatchers ) | ||
.fromElement( modelElement ) | ||
.toElement( () => ViewContainerElement.fromViewDefinition( viewDefinition ) ); | ||
} | ||
|
||
/** | ||
* Helper for creating view to model converter from view to model element. It will convert also all matched view elements defined in | ||
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. The first sentence makes no sense. "view to model converter" and then "from view to model"? |
||
* `acceptAlso` property. The `model` property is used as model element name. | ||
* | ||
* Conversion from model to view might be defined as simple one to one conversion: | ||
* | ||
* viewToModelElement( { model: 'heading1', view: 'h1' }, [ dispatcher ] ); | ||
* | ||
* As a full-flavored definition: | ||
* | ||
* viewToModelElement( { | ||
* model: 'heading1', | ||
* view: { | ||
* name: 'p', | ||
* attributes: { | ||
* 'data-heading': 'true' | ||
* }, | ||
* // it might require to define higher priority for elements matched by other features | ||
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. Upper case and period.
|
||
* priority: 'high' | ||
* } | ||
* }, [ dispatcher ] ); | ||
* | ||
* or with `acceptAlso` property to match many elements: | ||
* | ||
* viewToModelElement( { | ||
* model: 'heading1', | ||
* view: { | ||
* name: 'h1' | ||
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 using the object syntax in this case? |
||
* }, | ||
* acceptAlso: [ | ||
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. Maybe 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. The The So TL;DR: I don't think so ;) 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 mean view as array only for 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. Or I don't understand something. Maybe exactly the same definition will be used for both conversions. 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. But the configuration (like in heading) would be one: return VirtualTestEditor
.create( {
plugins: [ HeadingEngine ],
heading: {
options: [
{ model: 'paragraph', title: 'paragraph' },
{
model: 'heading1',
view: 'h1',
acceptsAlso: [
{
name: 'p',
attributes: { 'data-heading': 'h1' },
priority: 'high'
}
],
title: 'User H1'
}
]
}
} ) 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. Ok, makes sense now :) 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. And also the feature would use it as: |
||
* { name: 'p', attributes: { 'data-heading': 'level1' }, priority: 'high' }, | ||
* { name: 'h2', class: 'heading-main' }, | ||
* { name: 'div', style: { 'font-weight': 'bold', font-size: '24px' } } | ||
* ] | ||
* }, [ dispatcher ] ); | ||
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. Please use a real dispatchers here – I mean, something like |
||
* | ||
* Above example will convert such existing HTML content: | ||
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. The above |
||
* | ||
* <h1>A heading</h1> | ||
* <h2 class="heading-main">Another heading</h2> | ||
* <p data-heading="level1">Paragraph-like heading</p> | ||
* <div style="font-size:24px; font-weigh:bold;">Another non-semantic header</div> | ||
* | ||
* into `heading1` model element so after rendering it the output HTML will be cleaned up: | ||
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. Again - avoid using "HTML". 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. Ooops. I haven't noticed that you mention here retrieving the data back. So it makes sense to call it HTML. But at the same time, maybe it's better to only mention here what ends up in the model. It will be clearer where and when the unification happen. |
||
* | ||
* <h1>A heading</h1> | ||
* <h1>Another heading</h1> | ||
* <h1>Paragraph-like heading</h1> | ||
* <h1>Another non-semantic header</h1> | ||
* | ||
* @param {module:engine/conversion/configurationdefinedconverters~ConverterDefinition} definition A conversion configuration. | ||
* @param {Array.<module:engine/conversion/viewconversiondispatcher~ViewConversionDispatcher>} dispatchers | ||
*/ | ||
export function viewToModelElement( definition, dispatchers ) { | ||
const { model: modelElement, viewDefinitions } = parseConverterDefinition( definition ); | ||
|
||
const converter = prepareViewConverter( dispatchers, viewDefinitions ); | ||
|
||
converter.toElement( modelElement ); | ||
} | ||
|
||
/** | ||
* Helper for creating model to view converter from model's attribute | ||
* to {@link module:engine/view/attributeelement~AttributeElement AttributeElement}. The `acceptAlso` property is ignored. | ||
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. Don't specify the second param of |
||
* | ||
* You can define conversion as simple model element to view element conversion using simplified definition: | ||
* | ||
* modelAttributeToViewAttributeElement( 'bold', { | ||
* model: 'true', | ||
* view: 'strong', | ||
* }, [ dispatcher ] ); | ||
* | ||
* Or defining full-flavored view object: | ||
* | ||
* modelAttributeToViewAttributeElement( 'fontSize' { | ||
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 comma. |
||
* model: 'big', | ||
* view: { | ||
* name: 'span', | ||
* styles: { | ||
* 'font-size': '1.2em' | ||
* } | ||
* }, | ||
* }, [ dispatcher ] ); | ||
* | ||
* Above will generate an HTML tag for model's attribute `fontSize` with a `big` value set: | ||
* | ||
* <span style="font-size:1.2em;">...</span> | ||
* | ||
* @param {String} attributeName Attribute name from which convert. | ||
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. "to convert" or something even more understandable "The name of the model attribute which should be converted." |
||
* @param {module:engine/conversion/configurationdefinedconverters~ConverterDefinition} definition A conversion configuration. | ||
* @param {Array.<module:engine/conversion/modelconversiondispatcher~ModelConversionDispatcher>} dispatchers | ||
*/ | ||
export function modelAttributeToViewAttributeElement( attributeName, definition, dispatchers ) { | ||
const { model: attributeValue, viewDefinition } = parseConverterDefinition( definition ); | ||
|
||
buildModelConverter() | ||
.for( ...dispatchers ) | ||
.fromAttribute( attributeName ) | ||
.toElement( value => { | ||
if ( value != attributeValue ) { | ||
return; | ||
} | ||
|
||
return AttributeElement.fromViewDefinition( viewDefinition ); | ||
} ); | ||
} | ||
|
||
/** | ||
* Helper for creating view to model converter from view to model attribute. It will convert also all matched view elements defined in | ||
* `acceptAlso` property. The `model` property is used as model's attribute value to match. | ||
* | ||
* Conversion from model to view might be defined as simple one to one conversion: | ||
* | ||
* viewToModelAttribute( 'bold', { model: true, view: 'strong' }, [ dispatcher ] ); | ||
* | ||
* As a full-flavored definition: | ||
* | ||
* viewToModelAttribute( 'fontSize', { | ||
* model: 'big', | ||
* view: { | ||
* name: 'span', | ||
* style: { | ||
* 'font-size': '1.2em' | ||
* } | ||
* } | ||
* }, [ dispatcher ] ); | ||
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. Something's wrong with indentation here. |
||
* | ||
* or with `acceptAlso` property to match many elements: | ||
* | ||
* viewToModelAttribute( 'fontSize', { | ||
* model: 'big', | ||
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 don't like the fact that attirbute name is passed as a separate parameter (what also @jodator notice in the PR description). It is also strange that in one case At, the same time I undrestand that for feature configuration it is the best, because feature knows what is the name of the attribute. However, I would go here with: model: {
attributes: { 'fontSize': 'big' }
} This is configuration of the conversion building tool. The plugin configuration does not need to be the same. Plugin can do some transformation adding obvious data to it, to make its configuration as simple as possible. However, this config will not fit all features need and we should not try to make it 1-1 with the config. |
||
* view: { | ||
* name: 'span', | ||
* class: 'text-big' | ||
* }, | ||
* acceptAlso: [ | ||
* { name: 'span', attributes: { 'data-size': 'big' } }, | ||
* { name: 'span', class: [ 'font', 'font-huge' ] }, | ||
* { name: 'span', style: { font-size: '18px' } } | ||
* ] | ||
* }, [ dispatcher ] ); | ||
* | ||
* Above example will convert such existing HTML content: | ||
* | ||
* <p>An example text with some big elements: | ||
* <span class="text-big>one</span>, | ||
* <span data-size="big>two</span>, | ||
* <span class="font font-huge>three</span>, | ||
* <span style="font-size:18px>four</span> | ||
* <p> | ||
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 |
||
* | ||
* into `fontSize` model attribute with 'big' value set so after rendering it the output HTML will be cleaned up: | ||
* | ||
* <p>An example text with some big elements: | ||
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. By convention, "An example..." should be in a new line. |
||
* <span class="text-big>one</span>, | ||
* <span class="text-big>two</span>, | ||
* <span class="text-big>three</span>, | ||
* <span class="text-big>four</span> | ||
* <p> | ||
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 |
||
* | ||
* @param {String} attributeName Attribute name to which convert. | ||
* @param {module:engine/conversion/configurationdefinedconverters~ConverterDefinition} definition A conversion configuration. | ||
* @param {Array.<module:engine/conversion/viewconversiondispatcher~ViewConversionDispatcher>} dispatchers | ||
*/ | ||
export function viewToModelAttribute( attributeName, definition, dispatchers ) { | ||
const { model: attributeValue, viewDefinitions } = parseConverterDefinition( definition ); | ||
|
||
const converter = prepareViewConverter( dispatchers, viewDefinitions ); | ||
|
||
converter.toAttribute( () => ( { | ||
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. Are we fine with this notation or we prefer a 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 might saw that in code so any advice on team-preferred code style is welcome :) |
||
key: attributeName, | ||
value: attributeValue | ||
} ) ); | ||
} | ||
|
||
// Prepares a {@link module:engine/conversion/configurationdefinedconverters~ConverterDefinition definition object} for building converters. | ||
// | ||
// @param {module:engine/conversion/configurationdefinedconverters~ConverterDefinition} definition An object that defines view to model | ||
// and model to view conversion. | ||
// @returns {Object} | ||
function parseConverterDefinition( definition ) { | ||
const model = definition.model; | ||
const view = definition.view; | ||
|
||
const viewDefinition = typeof view == 'string' ? { name: view } : view; | ||
|
||
const viewDefinitions = definition.acceptsAlso ? definition.acceptsAlso : []; | ||
|
||
viewDefinitions.push( viewDefinition ); | ||
|
||
return { model, viewDefinition, viewDefinitions }; | ||
} | ||
|
||
// Helper method for preparing a view converter from passed view definitions. | ||
// | ||
// @param {Array.<module:engine/conversion/viewconversiondispatcher~ViewConversionDispatcher>} dispatchers | ||
// @param {Array.<module:engine/view/viewelementdefinition~ViewElementDefinition>} viewDefinitions | ||
// @returns {module:engine/conversion/buildviewconverter~ViewConverterBuilder} | ||
function prepareViewConverter( dispatchers, viewDefinitions ) { | ||
const converter = buildViewConverter().for( ...dispatchers ); | ||
|
||
for ( const viewDefinition of viewDefinitions ) { | ||
converter.from( definitionToPattern( viewDefinition ) ); | ||
|
||
if ( viewDefinition.priority ) { | ||
converter.withPriority( viewDefinition.priority ); | ||
} | ||
} | ||
|
||
return converter; | ||
} | ||
|
||
// Converts viewDefinition to a matcher pattern. | ||
// | ||
// @param {module:engine/view/viewelementdefinition~ViewElementDefinition} viewDefinition | ||
// @returns {module:engine/view/matcher~Pattern} | ||
function definitionToPattern( viewDefinition ) { | ||
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. Instead if translating syntaxes, we should make them the same (change matcher pattern to fit definition pattern). |
||
const name = viewDefinition.name; | ||
const classes = viewDefinition.classes; | ||
const styles = viewDefinition.styles; | ||
const attributes = viewDefinition.attributes; | ||
|
||
const pattern = { name }; | ||
|
||
if ( classes ) { | ||
pattern.class = classes; | ||
} | ||
|
||
if ( styles ) { | ||
pattern.style = styles; | ||
} | ||
|
||
if ( attributes ) { | ||
pattern.attribute = attributes; | ||
} | ||
|
||
return pattern; | ||
} | ||
|
||
/** | ||
* Defines conversion details. | ||
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 links to where is all this used. |
||
* | ||
* @typedef {Object} ConverterDefinition | ||
* @property {String} model Defines to model conversion. When using to element conversion | ||
* ({@link module:engine/conversion/configurationdefinedconverters~viewToModelElement} | ||
* and {@link module:engine/conversion/configurationdefinedconverters~modelElementToViewContainerElement}) | ||
* it defines element name. When using to attribute conversion | ||
* ({@link module:engine/conversion/configurationdefinedconverters~viewToModelAttribute} | ||
* and {@link module:engine/conversion/configurationdefinedconverters~modelAttributeToViewAttributeElement}) | ||
* it defines attribute value to which it is converted. | ||
* @property {String|module:engine/view/viewelementdefinition~ViewElementDefinition} view Defines model to view conversion and is also used | ||
* in view to model conversion pipeline. | ||
* @property {Array.<module:engine/view/viewelementdefinition~ViewElementDefinition>} acceptAlso An array with all matched elements that | ||
* view to model conversion should also accepts. | ||
*/ |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -712,6 +712,48 @@ export default class Element extends Node { | |
( attributes == '' ? '' : ` ${ attributes }` ); | ||
} | ||
|
||
/** | ||
* Creates element instance from provided viewElementDefinition. | ||
* | ||
* @param {module:engine/view/viewelementdefinition~ViewElementDefinition} viewElementDefinition | ||
* @returns {Element} | ||
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. Nope |
||
*/ | ||
static fromViewDefinition( viewElementDefinition ) { | ||
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. Misses a verb and repeating "view" isn't necessary IMO. 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. Note that soon view writer will be the only proper way of changing model, so such methods will have to be moved. |
||
const attributes = {}; | ||
|
||
const classes = viewElementDefinition.classes; | ||
|
||
if ( classes ) { | ||
attributes.class = Array.isArray( classes ) ? classes.join( ' ' ) : classes; | ||
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. Same as with styles below. |
||
} | ||
|
||
const stylesObject = viewElementDefinition.styles; | ||
|
||
if ( stylesObject ) { | ||
attributes.style = toStylesString( stylesObject ); | ||
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. Do you guys think that we could avoid this stringification? E.g. by setting styles after creating this element (then, view Uncareful stringification is unsafe. |
||
} | ||
|
||
const attributesObject = viewElementDefinition.attributes; | ||
|
||
if ( attributesObject ) { | ||
for ( const key in attributesObject ) { | ||
attributes[ key ] = attributesObject[ key ]; | ||
} | ||
} | ||
|
||
return new this( viewElementDefinition.name, attributes ); | ||
|
||
function toStylesString( stylesObject ) { | ||
const styles = []; | ||
|
||
for ( const key in stylesObject ) { | ||
styles.push( key + ':' + stylesObject[ key ] ); | ||
} | ||
|
||
return styles.join( ';' ); | ||
} | ||
} | ||
|
||
/** | ||
* Returns block {@link module:engine/view/filler filler} offset or `null` if block filler is not needed. | ||
* | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
/** | ||
* @license Copyright (c) 2003-2017, CKSource - Frederico Knabben. All rights reserved. | ||
* For licensing, see LICENSE.md. | ||
*/ | ||
|
||
/** | ||
* @module engine/view/viewelementdefinition | ||
*/ | ||
|
||
/** | ||
* An object defining view element used for defining elements for 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. Some examples and links to places where this is used will help. |
||
* | ||
* @typedef {Object} module:engine/view/viewelementdefinition~ViewElementDefinition | ||
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 don't know if it wouldn't be easier to define here that an element may be defined by a single string. Otherwise, everywhere this is used you'd need to write:
And you'll need to check the type of this property before deciding whether to use |
||
* | ||
* @property {String} name View element name. | ||
* @property {String|Array.<String>} [classes] Class name or array of class names to match. Each name can be | ||
* provided in a form of string. | ||
* @property {Object} [styles] Object with key-value pairs representing styles to match. Each object key | ||
* represents style name. Value under that key must be a string. | ||
* @property {Object} [attributes] Object with key-value pairs representing attributes to match. Each object key | ||
* represents attribute name. Value under that key must be a string. | ||
*/ |
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.
This file is named incorrectly. If it doesn't export a variable called
configurationDefinedConverters
then these are 3 separate words and hence the file should be calledconfiguration-defined-converters.js
.I'm also unsure about naming this file after "configuration". What if I just want to use these helpers in my feature (which has no configuration)? Can we think about a different name?
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.
definition-based-converters.js
? :DThere 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.
Or we need to find some proper name for these definitions. Something distinctive that we could use all around the code. Conversion specs, conversion maps, conversion rules, conversion... dunno. None of these is better than conversion definitions.
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.
Yeah... I go with that
definition-based-converts
and other cleanups. We can review it once again after I make all the requested changes.