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

Introduced DocumentSelection#event:change:marker #1818

Merged
merged 5 commits into from
Feb 6, 2020
Merged

Introduced DocumentSelection#event:change:marker #1818

merged 5 commits into from
Feb 6, 2020

Conversation

scofalik
Copy link
Contributor

Suggested merge commit message (convention)

Feature: Introduced DocumentSelection#event:change:marker. Closes ckeditor/ckeditor5#6133.

@coveralls
Copy link

coveralls commented Jan 24, 2020

Coverage Status

Coverage remained the same at 100.0% when pulling 5822fd4 on i/6133 into 48bea53 on master.

@@ -442,6 +442,50 @@ describe( 'DocumentSelection', () => {

expect( selection.markers.map( marker => marker.name ) ).to.have.members( [ 'marker' ] );
} );

it( 'should fire change:marker event when selection markers change', () => {
Copy link
Member

Choose a reason for hiding this comment

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

I don't like that this is a single test. In the code I can see at least 4 or 5 scenarios. setTo, setFocus, changed not being set to true, being set to true by the first loop and by the second loop. 5 cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've split this test into multiple unit tests and added a few more. I hope you will be satisfied ;).

@Reinmar Reinmar merged commit 5106014 into master Feb 6, 2020
@Reinmar Reinmar deleted the i/6133 branch February 6, 2020 11:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Introduce DocumentSelection#change:marker
3 participants