-
Notifications
You must be signed in to change notification settings - Fork 351
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
[Interactive Graph Editor] Remove the use of graphKey for remounting #1385
Conversation
Size Change: -153 B (-0.02%) Total Size: 849 kB
ℹ️ View Unchanged
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1385 +/- ##
==========================================
+ Coverage 69.82% 70.96% +1.14%
==========================================
Files 493 497 +4
Lines 104323 104387 +64
Branches 7522 11254 +3732
==========================================
+ Hits 72842 74078 +1236
+ Misses 31301 30309 -992
+ Partials 180 0 -180
... and 145 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
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.
LGTM! Thanks for debugging this issue and fixing it :)
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @khanacademy/perseus@24.2.0 ### Minor Changes - [#1386](#1386) [`5fdbeb980`](5fdbeb9) Thanks [@benchristel](https://github.com/benchristel)! - Add `mafs.point` flag to ApiOptions type ### Patch Changes - [#1388](#1388) [`94067d752`](94067d7) Thanks [@nishasy](https://github.com/nishasy)! - [Interactive Graph: Circle] Add a key prop to the circle drag handle ## @khanacademy/perseus-editor@7.0.2 ### Patch Changes - [#1385](#1385) [`30f898c44`](30f898c) Thanks [@nishasy](https://github.com/nishasy)! - [Interactive Graph Editor] Remove the use of graphKey for remounting - Updated dependencies \[[`5fdbeb980`](5fdbeb9), [`94067d752`](94067d7)]: - @khanacademy/perseus@24.2.0 Author: khan-actions-bot Reviewers: benchristel Required Reviewers: Approved By: benchristel Checks: ✅ codecov/project, ✅ codecov/patch, ✅ Upload Coverage (ubuntu-latest, 20.x), ⏭️ Publish npm snapshot, ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Jest Coverage (ubuntu-latest, 20.x), 🚫 Upload Coverage, ✅ gerald, ⏭️ Publish npm snapshot, 🚫 Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), 🚫 Check builds for changes in size (ubuntu-latest, 20.x), 🚫 Cypress (ubuntu-latest, 20.x), 🚫 Check for .changeset entries for all changed files (ubuntu-latest, 20.x), 🚫 Jest Coverage (ubuntu-latest, 20.x), ✅ gerald Pull Request URL: #1387
Summary:
It turns out that the use of
graphKey
for remounting ininteractive-graph-editor.tsx
is redundant, since this is also beingdone by the
useEffect
inmafs-graph.tsx
.graphKey
was also causing a warning to show up in the logs, possiblyfrom a remounting loop.
Here,
graphKey
is being removed altogether to stop the remounting issue.More info on Slack
Issue: none
Test plan:
Storybook