-
Notifications
You must be signed in to change notification settings - Fork 40
T/1198: ViewElementConfig with helper converters. #1205
Conversation
…efinition interface.
So the {
model: {
attributeName: 'foo',
attributeValue: 'bar'
}
}
// or
{
model: {
element: 'baz'
}
} ...but it could lead to mis configuration for feature that expects attribute to have the same name. So after second thought I'm for leaving |
* view: { | ||
* name: 'h1' | ||
* }, | ||
* acceptAlso: [ |
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.
Maybe view
could be an array
in case of multiple 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.
The view
option defines default model->view conversion and it should always be one as it would be impossible to convert one model value to many (random?) view definitions ;).
The acceptAlso
is used as a way to convert existing markup to model in view->model conversion. As in this example you always want to convert to h1
element but when loading content you want to map other elements to heading1
also.
So TL;DR: I don't think so ;)
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 mean view as array only for view->model
conversion. I wouldn't be afraid that someone will try to use it for converting to multiple or random element in case of model->view
conversion.
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.
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 comment
The 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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
And also the feature would use it as:
@@ -0,0 +1,310 @@ | |||
/** |
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 called configuration-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
? :D
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.
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.
* }, | ||
* }, [ dispatcher ] ); | ||
* | ||
* Above will generate an HTML tag: |
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 following DOM element:"
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, isn't it a view element?
* Helper for creating model to view converter from model's element | ||
* to {@link module:engine/view/containerelement~ContainerElement ViewContainerElement}. The `acceptAlso` property is ignored. | ||
* | ||
* 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 comment
The 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".
|
||
/** | ||
* 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 comment
The 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.
} | ||
|
||
/** | ||
* 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 comment
The 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"?
* 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 comment
The reason will be displayed to describe this comment to others. Learn more.
Upper case and period.
You may need to use a high-priority listener to catch elements which are handled by other (usually – more generic) converters too.
* viewToModelElement( { | ||
* model: 'heading1', | ||
* view: { | ||
* name: 'h1' |
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.
Why using the object syntax in this case?
* ] | ||
* }, [ dispatcher ] ); | ||
* | ||
* Above example will convert such existing HTML content: |
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 above
an existing view elements
src/view/element.js
Outdated
* 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 comment
The reason will be displayed to describe this comment to others. Learn more.
Nope
src/view/element.js
Outdated
const stylesObject = viewElementDefinition.styles; | ||
|
||
if ( stylesObject ) { | ||
attributes.style = toStylesString( stylesObject ); |
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.
Do you guys think that we could avoid this stringification? E.g. by setting styles after creating this element (then, view Element
should then have setStyles()
method)?
Uncareful stringification is unsafe.
src/view/element.js
Outdated
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Same as with styles below.
* <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 comment
The 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 comment
The 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.
|
||
/** | ||
* 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 comment
The reason will be displayed to describe this comment to others. Learn more.
Don't specify the second param of {@link}
because what you did here is actually breaking the link. It should look like: AttributeElement
but you made it AttributeElement. If you won't specify the second param it will be rendered correctly. Specify the second param only if you want to write something custom like config.foo.bar
(which would be better than the default bar
). Also, make sure to always wrap code names in backticks.
….createFromDefinition()` and use methods for adding styles & classes.
Most things were cleaned up. Remainig are:
Ad. 1. It might be a follow up - but I can do it rather quickly now. Full: const elementConfig = {
view: 'h1',
model: {
element: 'heading1'
}
};
const attributeConfig = {
view: { style: { 'font-size': 'big' } },
model: {
attribute: 'fontSize',
value: 'big'
}
}; Current: const elementConfig = {
view: 'h1',
model: 'heading1'
};
const attributeConfig = {
view: { style: { 'font-size': 'big' } },
model: 'big' // A feature knows on what attribute it works ('fontSize')
}; And after looking how features will be configured it would be a lot easier to change only I'm still for using |
If we can postpone it I'd rather do it later. Let's have features unblocked asap.
Exactly. It's enough to have Still, I'd just progress with what we have now. All this will be reviewed once again soon anyway. |
|
||
const viewDefinition = typeof view == 'string' ? { name: view } : view; | ||
|
||
const viewDefinitions = Array.from( definition.acceptsAlso ? definition.acceptsAlso : [] ); |
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.
Calling 2 variables in so similar way is asking for troubles. Why not leave "view" or "acceptsAlso"?
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.
As, I see that you adds viewDefinition
to viewDefinitions
. Still, I would call it better: fromView
and toView
(or sourceView
/targetView
).
// @param {module:engine/conversion/definition-based-converters~ConverterDefinition} definition An object that defines view to model | ||
// and model to view conversion. | ||
// @returns {Object} | ||
function parseConverterDefinition( definition ) { |
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 function gets JSON and return JSON, it parse nothing. It should be called "normaliseConverterDefinition".
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.
That's the word I was seeking then :)
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.
normalizeConverterDefinition()
in AmE.
src/view/element.js
Outdated
* @param {module:engine/view/viewelementdefinition~ViewElementDefinition} viewElementDefinition | ||
* @returns {module:engine/view/element~Element} | ||
*/ | ||
static createFromDefinition( viewElementDefinition ) { |
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.
Since this method will be removed from the view element I would put it in definition-based-converters.js
.
A agree with @Reinmar that there is no sense to do a deep review at this point. I only left a few comments to the code. In fact, the only thing I don't like in the current definition for features is the |
If so we should then be consequent and ad two: Also I don't see how those configuration would be mixed in one feature. |
I talked with @jodator and realized that in fact, users do not need to know if they are defining element names or attribute values. For headings, which is the only case for element names so far, TL;DR: |
TL;DR: I win 😜 |
…to definition-based-converters as a private method.
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.
It looks good for me. I leave it to @Reinmar for the final check and closing it.
Unfortunatelly I've found one bug. So please hold back this PR :( |
…buteElement as an array.
So the but was when changing attribute value from 'a' to 'b' in one step. I've caught it when dealing with I've changed |
Suggested merge commit message (convention)
Enhancement: Introduce ViewElementDefinition. Closes ckeditor/ckeditor5#4216.
Additional information
I'd love to hear about this implementation as I'm not sure if it's the right way to do this. Basically the most interesting file would be
configurationdefinedconverters.js
which has 4 helpers for defining V->M and M->V converters for bothContainerElement
andAttributeElmeent
. All methods have docs so please check the usage first.What I'm not sure about is mostly naming of that file. The other thing is that I've used other helpers that already existed
buildModelConverter
andbuildViewConverter
to build upon this helper methods.What I'm not sure right now is how to pass
ConversionDefinition
object. Right now I pass config directly as it should be compatible withConversionDefinition
object as I've used option 2 from this as @pjasiun suggested. Maybe it would be nicer to group all those required properties on one object?The other thins is the
model
part and how those util method accepts them as forElement
I've usedmodel
string directly whereas forAttribute
conversion I've used it asattributeValue
as you passattributeName
as method parameter. The rationale behind this was how those two features were structured. In Heading there were many possible elements (hence manymodel
attributes) while in Font I had only one attribute name (fontSize
) with many size options as different values.To see both in action check (or checkout
t/ckeditor5-font/2
branch on ckeditor5 repo):Element
conversion: heading branch, example testAttribute
conversion: font branch and example configuration tests.