Skip to content
This repository has been archived by the owner on Feb 6, 2023. It is now read-only.

Improve getCharacterRemovalRange.js #1645

Conversation

matedeilo
Copy link

Summary
PR for issue 1116
#1116

Test Plan
No tests added.

Copy link
Contributor

@flarnie flarnie left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

It's close, but I have one concern.

Since there are two boolean params, we have 4 possible cases to cover, not just 2. Specifically; getEntityRemovalRangeAtStart passes 'true, undefined' and the two calls it is replacing pass 'false, true' and 'true, false' respectively, so this could change the logic of what happens.

This code isn't well tested so please do some manual testing - I think you could manually test by opening the entity example in your browser and make selections that overlap entities, then hit 'delete'.

https://github.com/facebook/draft-js/blob/master/examples/draft-0-10-0/entity/entity.html

@flarnie
Copy link
Contributor

flarnie commented Feb 9, 2018

Or even better, add some tests? :)

@matedeilo
Copy link
Author

I will add some test, thnks for the advice.

@flarnie
Copy link
Contributor

flarnie commented Mar 2, 2018

Since this has been open for a while with no updates I'm closing - feel free to reopen when the change requests have been addressed. Thanks!

@flarnie flarnie closed this Mar 2, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants