-
Notifications
You must be signed in to change notification settings - Fork 40
T/1556: Expose conversion utilities #1613
Conversation
ps.: The code works with all repos checked out as in CKEditor5 (the core is crucial as it adds proper helpers to dispatcher groups). |
You can sort it, we used to the fact that
Let's do it in a separate ticket. |
this.for( 'upcast' ).add( | ||
upcastElementToAttribute( { | ||
this.for( 'upcast' ) | ||
.elementToAttribute( { | ||
view, | ||
model, | ||
converterPriority: definition.priority |
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 methods have the same names now, shouldn't they have the same parameters names too?
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 converterPriority
vs priority
, in conversion.for( 'downcast' ).attributeToElement
vs conversion.attributeToElement
.
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.
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 more like a bug. There is even an example in the documentation where converterPriority
is used. Also docs says that type of definition is ConverterDefinition
which uses converterPriority
there is nothing about definition.priority
property so I believe it should be simply:
converterPriority: definition.priority | |
converterPriority: definition.converterPriority |
@pjasiun I like the
this.conversion.register(
'downcast',
new DowncastHelpers( [
this.editing.downcastDispatcher,
this.data.downcastDispatcher
]
);
this.conversion.register( 'upcast', new UpcastHelpers( this.data.upcastDispatcher ) ); Forget about this: 👇 Also I'd move Edit: ☝️ or not... there are many private method used in both helpers and converters. but the name should be different, IDK... since the this.conversion.register( 'upcast', new UpcastDispatchers( this.data.upcastDispatcher ) ); so |
I agree with the first half, up to "Also" :) @jodator said that I should not care about the second half ;) |
@pjasiun done |
Do we need all of these exports in downcast helpers? (see https://github.com/ckeditor/ckeditor5-engine/issues/1306#issuecomment-378870980). It would be great to expose only these methods:
When it comes to testing, the rest of them should have only integrational tests. |
…htElement() and removeHighlight() from API.
Suggested merge commit message (convention)
Enhancement: Expose conversion utilities. Closes ckeditor/ckeditor5#4428.
BREAKING CHANGE: The
conversion.register()
method now accepts single options object as a parameter.BREAKING CHANGE: The
downcastElementToElement()
helper was removed from public API. Useconversion.for( 'downcast' ).elementToElement()
instead.BREAKING CHANGE: The
downcastAttributeToElement()
helper was removed from public API. Useconversion.for( 'downcast' ).attributeToElement()
instead.BREAKING CHANGE: The
downcastAttributeToAttribute()
helper was removed from public API. Useconversion.for( 'downcast' ).attributeToAttribute()
instead.BREAKING CHANGE: The
downcastMarkerToElement()
helper was removed from public API. Useconversion.for( 'downcast' ).markerToElement()
instead.BREAKING CHANGE: The
downcastMarkerToHighlight()
helper was removed from public API. Useconversion.for( 'downcast' ).markerToHighlight()
instead.BREAKING CHANGE: The
upcastElementToElement()
helper was removed from public API. Useconversion.for( 'upcast' ).elementToElement()
instead.BREAKING CHANGE: The
upcastElementToAttribute()
helper was removed from public API. Useconversion.for( 'upcast' ).elementToAttribute()
instead.BREAKING CHANGE: The
upcastAttributeToAttribute()
helper was removed from public API. Useconversion.for( 'upcast' ).attributeToAttribute()
instead.BREAKING CHANGE: The
upcastElementToMarker()
helper was removed from public API. Useconversion.for( 'upcast' ).elementToMarker()
instead.BREAKING CHANGE: The
insertUIElement()
andremoveUIElement()
downcast converters were removed from public API. Useconversion.for( 'downcast' ).markerToElement()
instead.BREAKING CHANGE: The
highlightText()
,highlightElement()
andremoveHighlight()
downcast converters were removed from public API. Useconversion.for( 'downcast' ).markerToHighlight()
instead.BREAKING CHANGE: The
insertElement()
downcast converter was removed from public API. Useconversion.for( 'downcast' ).elementToElement()
instead.BREAKING CHANGE: The
changeAttribute()
downcast converter was removed from public API. Useconversion.for( 'downcast' ).attributeToAttribute()
instead.Additional information
elementToElement
attributeToElement
attributeToAttribute
markerToElement
markerToHighlight
elementToElement
elementToAttribute
attributeToAttribute
elementToMarker
@protected
and underscored.this.add()
internally to add a protected converter):Some questions:
convertText()
,convertToModelFragment()
etc.git blame
immediate history in those functions. Also in this file export and private methods are mixed - should we sort them aspublic export > protected export > internal/private
?