Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

T/1618: Merge *cast-selection-converters into *casthelpers #1628

Merged
merged 8 commits into from
Jan 2, 2019
Merged

Conversation

jodator
Copy link
Contributor

@jodator jodator commented Jan 2, 2019

Suggested merge commit message (convention)

Other: Merge *cast-selection-converters into *casthelpers. Closes ckeditor/ckeditor5#4455.

BREAKING CHANGE: The clearAttributes() downcast conversion helper was moved to module:engine/conversion/downcasthelpers.
BREAKING CHANGE: The convertCollapsedSelection() downcast conversion helper was moved to module:engine/conversion/downcasthelpers.
BREAKING CHANGE: The convertRangeSelection() downcast conversion helper was moved to module:engine/conversion/downcasthelpers.
BREAKING CHANGE: The convertSelectionChange() upcast conversion helper was moved to module:engine/conversion/upcasthelpers.


Additional information

@jodator jodator requested a review from pjasiun January 2, 2019 15:14
@pjasiun
Copy link

pjasiun commented Jan 2, 2019

I think they should be also marked as protected exports. Note that it is fine to use the protected code in tests and related files in the same module.

@pjasiun
Copy link

pjasiun commented Jan 2, 2019

Ach, I see that insertText, remove, etc. are not protected. Thet let me rethink it.

@jodator
Copy link
Contributor Author

jodator commented Jan 2, 2019

Ach, I see that insertText, remove, etc. are not protected. Thet let me rethink it

Yeah we might take a second look at those methods. It might be a case that those exports are needed because of dev-utils or use in controllers/editors/model but are not needed by an enddeveloper.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 37a981f on t/1618 into 1c7ef68 on master.

@jodator
Copy link
Contributor Author

jodator commented Jan 2, 2019

@pjasiun mostly here:

// Convert selection from the view to the model when it changes in the view.
this.listenTo( this.view.document, 'selectionChange', convertSelectionChange( this.model, this.mapper ) );
// Attach default model converters.
this.downcastDispatcher.on( 'insert:$text', insertText(), { priority: 'lowest' } );
this.downcastDispatcher.on( 'remove', remove(), { priority: 'low' } );
// Attach default model selection converters.
this.downcastDispatcher.on( 'selection', clearAttributes(), { priority: 'low' } );
this.downcastDispatcher.on( 'selection', convertRangeSelection(), { priority: 'low' } );
this.downcastDispatcher.on( 'selection', convertCollapsedSelection(), { priority: 'low' } );

@pjasiun pjasiun merged commit f041f28 into master Jan 2, 2019
@pjasiun pjasiun deleted the t/1618 branch January 2, 2019 16:02
@pjasiun
Copy link

pjasiun commented Jan 2, 2019

I was thinking about them for a while (and I even started writing a follow-up) and let's keep it the way it is. These helpers are not exposed in the editor API, you need an external import to use them. Also, I can imagine one who wants to build a tool similar to dev-utils based on them so it is fine as it is.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants