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

Proposal: Rename Range.includes to Range.intersects and add Range.surrounds to remove confusion #4283

Closed
thesunny opened this issue May 21, 2021 · 2 comments · Fixed by #5729

Comments

@thesunny
Copy link
Collaborator

thesunny commented May 21, 2021

Problem
Jason Tamulonis expressed that to him includes means that one Range fully includes another in Slack.

Anyone else feel like https://github.com/ianstormtaylor/slate/blame/main/packages/slate/src/interfaces/range.ts#L91 this should be an && and not an or? includes to me implies it includes the entire target not just has some overlap.

Andrew Herron added

This has come up a few times, iirc

My intuition is similar. The issue can be addressed by renaming the method for clarity and adding another method to handle the use case of what others were looking for.

Solution
Rename Range.includes to Range.intersects. This adds clarity as the language suggests that if any part of one range is in any part of the other, it shall return true, which is the behavior.

Add a method Range.surrounds which addresses the desired functionality of detecting that one range completely surrounds (or is equal to) the entire other range.

Alternatives
Improving the documentation is one step towards reducing confusion, which I will also do. I'll just add it since that's an easy win. Note: DONE https://docs.slatejs.org/api/locations/range#range-includes-range-range-target-path-or-point-or-range-boolean

Context

As this is a breaking change, @ianstormtaylor 

Thumbs up to approve, Thumbs down to disapprove proposal

@ianstormtaylor
Copy link
Owner

Sounds good! Should be implemented as a deprecation with tiny-warning first and can be removed in a later version that way without breaking in the interim.

@electroluxcode
Copy link
Contributor

electroluxcode commented Sep 24, 2024

agree with this proposal; the range.include parameter is confusing(any part of one range).this sounds like an intersection rather than an include. I am currently using the following code to replace this behavior.

const RangeInclude = (targetIncludeData, AllData) => {
	return Range.equals(Range.intersection(targetIncludeData, AllData), targetIncludeData)
}

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

Successfully merging a pull request may close this issue.

3 participants