Revert PR #1654 #1663
Merged
Revert PR #1654 #1663
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…)" This reverts commit c04d275.
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (99a52a9) and published it to npm. You Example: yarn add @khanacademy/perseus@PR1663If you are working in Khan Academy's webapp, you can run: ./dev/tools/bump_perseus_version.sh -t PR1663 |
|
Size Change: -156 B (-0.02%) Total Size: 863 kB
ℹ️ View Unchanged
|
nicolecomputer
approved these changes
Sep 24, 2024
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1663 +/- ##
==========================================
- Coverage 70.89% 70.68% -0.22%
==========================================
Files 562 587 +25
Lines 108291 112784 +4493
Branches 7981 12007 +4026
==========================================
+ Hits 76774 79717 +2943
- Misses 31330 33067 +1737
+ Partials 187 0 -187 see 1148 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Merged
benchristel
added a commit
that referenced
this pull request
Sep 24, 2024
benchristel
added a commit
that referenced
this pull request
Sep 27, 2024
benchristel
added a commit
that referenced
this pull request
Sep 27, 2024
This change was previously implemented in PR #1654, then reverted in #1663 due to a bug. This PR reimplements the change without the bug. The original PR summary follows. --- Previously, we were passing the entire Mafs graph state to onChange. Since the onChange callback data is used by the Interactive Graph widget editor to set the correct answer for the graph, a bunch of extra data like "hasBeenInteractedWith": true was getting saved in the correct field of the Widget data. This extra data is not used, so its presence in datastore is potentially confusing to future maintainers trying to understand the data. It might also cause bugs in the future - e.g. if some code merges it into another object in which those properties are meaningful. This PR ensures that we only save relevant data in the correct field of interactive graph widgets. Issue: none ## Test plan: `yarn dev; open http://localhost:5173` Play around with the graphs on the dev UI gallery page. In particular: - Unlimited point - Polygons with angle and side labels - Polygons with side snapping You should not observe any issues. Edit, save and publish an interactive graph widget in an exercise. As a learner, you should be able to complete the exercise successfully. Author: benchristel Reviewers: jeremywiebe, nicolecomputer Required Reviewers: Approved By: jeremywiebe Checks: ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ gerald Pull Request URL: #1664
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The linked PR caused a bug where certain interactive graph types would lose key features
after you interacted with them:
The cause was that the interactive graph widget was passing incomplete data to
its onChange callback. The Renderer stored this data in its state, overwriting the
original widget config. The borked data then got passed back down to the widget on the
next render, which caused the features to disappear.
Issue: none
Test plan:
yarn dev; open http://localhost:5173Turn on Mafs for the unlimited point and polygon graphs. Play with the graphs and
confirm that everything looks OK.