-
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
Omit extra properties from InteractiveGraph onChange arg #1654
Conversation
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
@@ -90,7 +91,7 @@ export type ChangeHandler = ( | |||
// perseus-all-package/widgets/grapher.jsx | |||
plot?: any; | |||
// Interactive Graph callback (see legacy: interactive-graph.tsx) | |||
graph?: InteractiveGraphState; | |||
graph?: PerseusGraphType; |
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.
This type was wrong - InteractiveGraphState is the internal state of our StatefulMafsGraph component - we don't want it to leak outside that component. PerseusGraphType
is what gets saved in the widget JSON.
@@ -1656,7 +1656,6 @@ class LegacyInteractiveGraph extends React.Component<Props, State> { | |||
|
|||
$(this.angle).on("move", () => { | |||
this.onChange({ | |||
// @ts-expect-error Type '{ coords: any; type: "angle"; showAngles?: boolean | undefined; allowReflexAngles?: boolean | undefined; angleOffsetDeg?: number | undefined; snapDegrees?: number | undefined; match?: "congruent" | undefined; }' is not assignable to type 'InteractiveGraphState | undefined'. |
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.
The incorrect type above was causing this error.
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (2e7e30a) and published it to npm. You Example: yarn add @khanacademy/perseus@PR1654 If you are working in Khan Academy's webapp, you can run: ./dev/tools/bump_perseus_version.sh -t PR1654 |
Size Change: +149 B (+0.02%) Total Size: 862 kB
ℹ️ View Unchanged
|
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.
TypeScript forces this function to be unfortunately verbose, but all it's doing is converting the circle point coords to center
and radius
, and passing the coords
and type
through for all the other graph types.
// the the parent InteractiveGraph. | ||
// | ||
// The transformed state is used in the interactive graph widget editor, to | ||
// set the `correct` field of the graph options. |
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.
@jeremywiebe @nishasy I'd especially like your feedback on this comment. Is this correct? Does anything besides the editor handle widgets' onChange
event?
I found some dead test prep code in webapp that was using onChange
, but that doesn't seem worth mentioning here since I believe it's about to be deleted. I also had a vague memory that we saved learners' progress to the backend on change, but that didn't seem to be the case when I tested - perhaps that feature was only in test prep.
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.
The renderer
itself sends an onChange
callback to widgets where it updates its state (widget state).
I also think that some widgets that accept user input send an onChange
event with updated prop values and expect to be re-rendered with these new values. For example, the label-image
widget does this (Github).
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.
So I don't think the editor is the only thing that uses onChange
. I am not sure as I've never dug through, but I've long wondered if we need such an all-encompassing onChange
handler type or if we could break it apart by widget.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1654 +/- ##
==========================================
- Coverage 70.67% 70.55% -0.12%
==========================================
Files 559 586 +27
Lines 107880 112437 +4557
Branches 5306 11368 +6062
==========================================
+ Hits 76240 79332 +3092
- Misses 31519 33105 +1586
+ Partials 121 0 -121 see 1145 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
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 could use a slight tweak to the comment (based on my comments in-line), but the code changes look good!
// the the parent InteractiveGraph. | ||
// | ||
// The transformed state is used in the interactive graph widget editor, to | ||
// set the `correct` field of the graph options. |
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.
The renderer
itself sends an onChange
callback to widgets where it updates its state (widget state).
I also think that some widgets that accept user input send an onChange
event with updated prop values and expect to be re-rendered with these new values. For example, the label-image
widget does this (Github).
// the the parent InteractiveGraph. | ||
// | ||
// The transformed state is used in the interactive graph widget editor, to | ||
// set the `correct` field of the graph options. |
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.
So I don't think the editor is the only thing that uses onChange
. I am not sure as I've never dug through, but I've long wondered if we need such an all-encompassing onChange
handler type or if we could break it apart by widget.
adbd077
to
2e7e30a
Compare
…)" This reverts commit c04d275.
The linked PR caused a bug where certain interactive graph types would lose key features after you interacted with them: - Polygon graphs would lose angle and side labels - Unlimited point graphs would only let you add a single point 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:5173` Turn on Mafs for the unlimited point and polygon graphs. Play with the graphs and confirm that everything looks OK. Author: benchristel Reviewers: nicolecomputer, #perseus Required Reviewers: Approved By: nicolecomputer Checks: ✅ codecov/patch, ❌ codecov/project, ✅ Upload Coverage (ubuntu-latest, 20.x), ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Jest Coverage (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), 🚫 Upload Coverage, ✅ gerald, ✅ Publish npm snapshot (ubuntu-latest, 20.x), 🚫 Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x), 🚫 Jest Coverage (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ gerald Pull Request URL: #1663
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
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 thecorrect
field ofthe 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 ofinteractive graph widgets.
Issue: none
Test plan:
Edit, save and publish an interactive graph widget in an exercise. As a
learner, you should be able to complete the exercise successfully.