8000 Fix SelectionState’s `hasEdgeWithin` by andrewbranch · Pull Request #1811 · facebookarchive/draft-js · GitHub
[go: up one dir, main page]

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

Fix SelectionState’s hasEdgeWithin #1811

Closed
wants to merge 4 commits into from
Closed

Fix SelectionState’s hasEdgeWithin #1811

wants to merge 4 commits into from

Conversation

andrewbranch
Copy link
Contributor

Fixes #1700, which includes a detailed description and reproduction of the issue.

Hopefully it's ok that I refactored the true/false assertions not to use snapshots; I found them confusing and tedious and figured it was probably just an artifact of migrating to Jest a long time ago.

@niveditc
Copy link
Contributor
niveditc commented Sep 8, 2018

@andrewbranch - thanks for putting up this fix. I won't get a chance to review until next weekend, but if you could rebase on master in the meanwhile, that would be a big help!

@andrewbranch
Copy link
Contributor Author

@niveditc done 👍

Copy link
Contributor
@niveditc niveditc left a comment

Choose a reason for hiding this comment

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

Looks good!

I checked the two current usages of hasEdgeWithin in the Draft core & both of them remain unchanged with this fix, so I think this should be an overall improvement since the logic is correct now :)

@niveditc
Copy link
Contributor
niveditc commented Oct 7, 2018

Next steps are that:

  • I'll import this and run FB's test stack.
  • Do a few sanity checks in the browser.
  • Provided the steps above are successful I'll merge it in tomorrow (Monday). In the slim chance that something breaks it's better to do a risky merge on a weekday than a weekend.

Copy link
@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

niveditc has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

jdecked pushed a commit to twitter-forks/draft-js that referenced this pull request Oct 9, 2019
Summary:
Fixes facebookarchive#1700, which includes a detailed description and reproduction of the issue.

Hopefully it's ok that I refactored the true/false assertions not to use snapshots; I found them confusing and tedious and figured it was probably just an artifact of migrating to Jest a long time ago.
Pull Request resolved: facebookarchive#1811

Differential Revision: D10234643

fbshipit-source-id: 531e45c8fe1708fb440240e1a7bd121cb126cd08
alicayan008 pushed a commit to alicayan008/draft-js that referenced this pull request Jul 4, 2023
Summary:
Fixes #1700, which includes a detailed description and reproduction of the issue.

Hopefully it's ok that I refactored the true/false assertions not to use snapshots; I found them confusing and tedious and figured it was probably just an artifact of migrating to Jest a long time ago.
Pull Request resolved: facebookarchive/draft-js#1811

Differential Revision: D10234643

fbshipit-source-id: 531e45c8fe1708fb440240e1a7bd121cb126cd08
aforismesen added a commit to aforismesen/draft-js that referenced this pull request Jul 12, 2024
Summary:
Fixes #1700, which includes a detailed description and reproduction of the issue.

Hopefully it's ok that I refactored the true/false assertions not to use snapshots; I found them confusing and tedious and figured it was probably just an artifact of migrating to Jest a long time ago.
Pull Request resolved: facebookarchive/draft-js#1811

Differential Revision: D10234643

fbshipit-source-id: 531e45c8fe1708fb440240e1a7bd121cb126cd08
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
0