-
-
Notifications
You must be signed in to change notification settings - Fork 81
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 action remove selection #348
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Left one minor comment. The tests look really good. I see you have an example where the target strictly contains the selection. In that vein, I'd add a test for the converse:
- Test where selection strictly contains target
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok nice tests thanks. Left a few more comments. I think I found a case we're missing
}); | ||
|
||
return { | ||
thatMark: targets.map((target) => target.selection), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think the that
mark would be more useful if it were the selections that got dropped? We could probably just leave it for now and see how it works in practice but just a thought
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can leave it like this and try it out. In most cases that is token you actually referenced. But I do agree that could potentially be useful.
Maybe we can use the source mark?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm not sure source mark is right here but interesting idea. Agreed tho let's just see if we ever actually want a way to refer to the removed selections in practice
Co-authored-by: Pokey Rule <pokey.rule@gmail.com>
…into removeSelection
Closes #95