T/1295 Further improvements to conversion helpers #1302
Conversation
…of conversion utils.
… converters do not handle passing element instance.
Fixed: Classes and styles are properly handled in downcast converters. Docs: Fixed converters docs. Changed: Support for passing element name in `downcastAttributeToAttribute` converter. Added: Introduced `conversion.ConverterDefinition` type. Added: Added priority handling in `conversion.Conversion` converters. Changed: Cleaned low-level converters after they can receive only element creator function as a parameter.
src/conversion/conversion.js
Outdated
| * Sets up converters between the model and the view which convert a model element to a view element (and vice versa). | ||
| * For example, model `<paragraph>Foo</paragraph>` is `<p>Foo</p>` in the view. | ||
| * | ||
| * `definition.model` is a `String` with a model element name to converter from/to. |
There was a problem hiding this comment.
I would put it togheter with the parameter desctription. When it is here, I ask myself why you tell me what model is, but not what view is.
There was a problem hiding this comment.
It's here because in the parameter definition specifies that input parameter is ConverterDefinition. I am not sure if it will be readable if such a long description would be put there.
There was a problem hiding this comment.
I know, but users will not know it. You can write that all other parameters are defined in ConverterDefinition, or move it after samples close to the parameter definition.
There was a problem hiding this comment.
I will move it lower.
src/conversion/conversion.js
Outdated
| * For example, model text node with data `"Foo"` and `bold` attribute is `<strong>Foo</strong>` in the view. | ||
| * | ||
| * `definition.model` parameter specifies what model attribute should be converted from/to. It can be a `{ key, value }` object | ||
| * describing attribute key and value to convert or a `String` specifying just attribute key (then `value` is set to `true`). |
src/conversion/conversion.js
Outdated
| * 'div', | ||
| * { | ||
| * // Any element with `display: block` style. | ||
| * name: /./, |
There was a problem hiding this comment.
I think we should improve it, like we did for attributes in https://github.com/ckeditor/ckeditor5-engine/issues/1239. It should be no name in this case.
There was a problem hiding this comment.
This is a tricky example, actually.
First of all, for Matcher purposes, specifying something like:
{
name: true,
class: ...
}
is just stupid, cause the name could be omitted. It says "any name" and every element has a name.
OTOH, the name is in upcast definition intentionally, so that name is returned in a match so it will be consumed.
I think that if we want to improve that, we should add name to matches object (the one returned by Matcher#match) in elementToElement and attributeToElement (so whenever a view element is converted).
There was a problem hiding this comment.
Okay I did as described above ☝️
src/conversion/conversion.js
Outdated
| * const size = Number( match[ 1 ] ); | ||
| * | ||
| * if ( size > 26 ) { | ||
| * return { name: true, style: [ 'font-size' ] }; |
There was a problem hiding this comment.
There should be a comment that this is information what should be consumed with the link to the full documentation. name: true looks complcated with no explanation.
There was a problem hiding this comment.
The comment might be helpful. I haven't added it cause, AFAIR, it is commented in MatcherPattern description.
src/conversion/conversion.js
Outdated
| * | ||
| * if ( viewElement.is( 'span' ) && fontWeight && /\d+/.test() && Number( fontWeight ) > 500 ) { | ||
| * return { | ||
| * name: true, |
| * return null; | ||
| * } | ||
| * } | ||
| * } ); |
There was a problem hiding this comment.
Thumbs up for...? :P
src/conversion/conversion.js
Outdated
| * For example, `<image src='foo.jpg'></image>` is converted to `<img src='foo.jpg'></img>` (same attribute key and value). | ||
| * | ||
| * `definition.model` parameter specifies what model attribute should be converted from/to. | ||
| * It can be a `{ key, values, [ name ] }` object or a `String`, which will be treated like `{ key: definition.model }`. |
There was a problem hiding this comment.
{ key, [ values ], [ name ] } (value is optional too)
There was a problem hiding this comment.
As we are at it, I wanted to ask you, whether it is better to name that property values or value (it is always an array).
|
IDK why this build: https://travis-ci.org/ckeditor/ckeditor5/builds/343343886 is still failing on docs, I've fixed all docs problems and pushed commits and I am clean on my local machine. |
…is upcast using conversion helpers.
Suggested merge commit message (convention)
Other: Introduced several improvements to conversion helpers. Closes ckeditor/ckeditor5#4275. Closes ckeditor/ckeditor5#4273. Closes ckeditor/ckeditor5#4272. Closes ckeditor/ckeditor5#4271. Closes ckeditor/ckeditor5#4270. Closes ckeditor/ckeditor5#4280.
Additional information
Basing and waiting for #1296. After #1296 is closed, I'll make PRs in other repositories related to this PR.