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

Feature: Introduced Writer#refreshMarker method. #1694

Merged
merged 3 commits into from
Mar 12, 2019
Merged

Feature: Introduced Writer#refreshMarker method. #1694

merged 3 commits into from
Mar 12, 2019

Conversation

oskarwrobel
Copy link
Contributor

@oskarwrobel oskarwrobel commented Mar 11, 2019

Suggested merge commit message (convention)

Feature: Added possibility to refresh the marker with no changes through Writer#updateMarker() method. Closes ckeditor/ckeditor5#4471.


Additional information

For example – encountered issues, assumptions you had to make, other affected tickets, etc.

@coveralls
Copy link

coveralls commented Mar 11, 2019

Coverage Status

Coverage increased (+3.0e-05%) to 99.977% when pulling 101c1ab on t/1649 into f4e4644 on master.

@pjasiun
Copy link

pjasiun commented Mar 11, 2019

We agreed to use writer.updateMarker( name ).

@scofalik scofalik requested review from scofalik and removed request for scofalik March 12, 2019 10:15
const marker = this._markers.get( markerName );

if ( !marker ) {
throw new CKEditorError( 'markers-refresh-marker-not-exists: Marker with provided name does not exists.' );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
throw new CKEditorError( 'markers-refresh-marker-not-exists: Marker with provided name does not exists.' );
throw new CKEditorError( 'markercollection-refresh-marker-not-exists: Marker with provided name does not exists.' );

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

throw new CKEditorError( 'markers-refresh-marker-not-exists: Marker with provided name does not exists.' );
}

const range = marker.getRange();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const range = marker.getRange();
const range = marker.getRange();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@@ -939,13 +939,38 @@ export default class Writer {
}

/**
* Adds or updates a {@link module:engine/model/markercollection~Marker marker}. Marker is a named range, which tracks
* Adds updates or refreshes a {@link module:engine/model/markercollection~Marker marker}. Marker is a named range, which tracks
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Adds updates or refreshes a {@link module:engine/model/markercollection~Marker marker}. Marker is a named range, which tracks
* Adds, updates or refreshes a {@link module:engine/model/markercollection~Marker marker}. Marker is a named range, which tracks

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

* changes in the document and updates its range automatically, when model tree changes. Still, it is possible to change the
* marker's range directly using this method.
*
* As the first parameter you can set marker name or instance. If none of them is provided, new marker, with a unique
* name is created and returned.
*
* As the second parameter you can set the new marker data or leave this parameter as empty what will only refresh
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* As the second parameter you can set the new marker data or leave this parameter as empty what will only refresh
* As the second parameter you can set the new marker data or leave this parameter as empty which will just refresh

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

it( 'should throw if marker does not exist', () => {
expect( () => {
markers._refresh( 'name' );
} ).to.throw( CKEditorError, 'markers-refresh-marker-not-exists: Marker with provided name does not exists.' );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
} ).to.throw( CKEditorError, 'markers-refresh-marker-not-exists: Marker with provided name does not exists.' );
} ).to.throw( CKEditorError, 'markercollection-refresh-marker-not-exists: Marker with provided name does not exists.' );

@scofalik scofalik merged commit cf56d90 into master Mar 12, 2019
@scofalik scofalik deleted the t/1649 branch March 12, 2019 10:41
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Trigger marker refresh
4 participants