Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added selection.isCollapsed method #818

Merged
merged 6 commits into from
Aug 30, 2017
Merged

Added selection.isCollapsed method #818

merged 6 commits into from
Aug 30, 2017

Conversation

Comandeer
Copy link
Member

What is the purpose of this pull request?

Task

Does your PR contain necessary tests?

All patches which change the editor code must include tests. You can always read more
on PR testing,
how to set the testing environment and
how to create tests
in the official CKEditor documentation.

This PR contains

  • Unit tests
  • Manual tests

What changes did you make?

I've added selection.isCollapsed method with appropriate unit tests.

closes #800.

@mlewand mlewand self-requested a review August 25, 2017 08:36
@mlewand mlewand changed the title Selection.isCollapsed Added selection.isCollapsed method Aug 25, 2017
@mlewand
Copy link
Contributor

mlewand commented Aug 25, 2017

Please check minor corrections in my two above commits. I find test case added in 6475902 redundant.

Instead this test Make fake table selection should ensure that ranges are not collapsed.

The fact whether or not multiple ranges/collapsed selection are correctly handled by isCollapsed is greatly covered in tests you have added to the tests/core/selection/selection.js suite.

So my suggestion is to add collapsed prop check for ranges in test case mentioned above, and remove freshly added test case.

Copy link
Contributor

@mlewand mlewand left a comment

Choose a reason for hiding this comment

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

Comments added in the PR.

@Comandeer Comandeer requested a review from mlewand August 28, 2017 09:36
@mlewand mlewand merged commit 66befd5 into master Aug 30, 2017
@mlewand mlewand deleted the t/800 branch August 30, 2017 08:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants