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

Bug: Range.includes(range, targetRange) missed for range fully inside targetRange case #4064

Closed
ulion opened this issue Jan 31, 2021 · 4 comments

Comments

@ulion
Copy link
Contributor

ulion commented Jan 31, 2021

Do you want to request a feature or report a bug?

report a bug.

What's the current behavior?

Range.includes(range, targetRange) does not cover range fully inside targetRange case, it will return false for this case. As discussed at: #3093 (comment)

Slate: since #3093 until now (0.60.2)
Browser: Not related
OS: Not related

What's the expected behavior?

return true

@BrentFarese
Copy link
Collaborator

BrentFarese commented Jan 31, 2021

This issue needs more description about what the actual bug is. Please add/elaborate.

If I'm thinking about what you're saying correctly, you are saying that Range.includes should return true if the range parameter is fully inside the target and both are Ranges? Does the range really include the target if the range is fully inside the target? If we change it to return true in that case, I might say the API name ought to be change to Range.overlaps or something, perhaps...

@ulion
Copy link
Contributor Author

ulion commented Jan 31, 2021

@BrentFarese The name is indeed not clear for this. depends on the function description in the comment Check if a range includes a path, a point or part of another range. if it's targeting the part of another range, the current implement is incomplete for the fully inside targetRange case.

@BrentFarese
Copy link
Collaborator

@BrentFarese The name is indeed not clear for this. depends on the function description in the comment Check if a range includes a path, a point or part of another range. if it's targeting the part of another range, the current implement is incomplete for the fully inside targetRange case.

Got it. Do you want to submit a PR? It seems like a relatively easy fix. Might just add a Range.includes(target, range) check and flip the order of arguments and then it would encompass what you want? We also need a test case(s) and I would say maybe we consider changing the name to Range.overlaps but that would be a breaking change so maybe not for now.

@ulion
Copy link
Contributor Author

ulion commented Jan 31, 2021

Let's say maybe we just leave it here for now, since Range.includes for target range is indeed not used by any popular slate library as far as I know. So, if any one want to use it for range includes check (indeed intersection as we see), he need fix it first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants