Add "static" toggle to Interactive Graph widget #1503

benchristel · 2024-08-08T19:46:58Z

Summary:

Some of our widgets (e.g. Radio) have a "static" mode which can be toggled on
in the exercise editor. When a widget is in static mode, the "answer" to the
widget is displayed, the widget is not graded, and the learner cannot alter
the widget's state.

Content creators need to be able to set interactive graphs to static mode so
that graphs can be added to hints, to demonstrate the solution steps for an
exercise. Previously, we implemented a special "hint mode" for interactive
graphs to accommodate this use case. Hint mode was based on a couple faulty
assumptions:

We did not clearly understand what static mode was or when it was turned on.
We confused static mode with the "read only" state of a widget, which occurs
when a learner has successfully answered the question. Similar to static mode,
the read-only state makes the widget non-interactable. However, while static
mode is configurable by content creators, read-only state is not; it is set
or unset dynamically, based on the learner's progress through an exercise.
We assumed that graphs in hints would always be non-interactable, so we
turned on hint mode automatically in hints (and turned it off in question
stems) rather than giving content creators a manual toggle. This assumption
turned out to be incorrect; we now know of at least one question where a hint
contains a graph that the learner is supposed to be able to manipulate. See:
https://khanacademy.slack.com/archives/C067UM1QAR4/p1723045477082029

Hint mode (which this feature replaces) was never released to content creators,
so we are okay to drop support for it.

This PR removes hint mode and adds a "static" toggle to the Interactive
Graph widget editor. It builds on work that @nicolecomputer did in
#1457. Her feature branch is merged into
this one.

Issue: LEMS-2251, LEMS-2053, LEMS-2054

Test plan:

Deploy this to a ZND
Create an exercise in the Test Everything course
Create a static interactive graph
Verify that the preview is non-interactable (with mouse and keyboard) when
the "static" toggle is on, and interactable when "static" is off.
Save and publish the exercise.
Go through the exercise as a learner and verify that you cannot manipulate
the static graphs.
Verify that the presence of a static graph does not affect how a question is
graded.

khan-actions-bot · 2024-08-08T19:47:15Z

Gerald

Required Reviewers

@Khan/perseus for changes to .changeset/gentle-dragons-hide.md, packages/perseus/src/hint-renderer.tsx, packages/perseus/src/renderer.tsx, packages/perseus/src/types.ts, packages/perseus/src/__tests__/renderer.test.tsx, packages/perseus/src/mixins/widget-prop-denylist.ts, packages/perseus/src/widgets/interactive-graph.tsx, packages/perseus-editor/src/__stories__/editor-page-with-storybook-preview.tsx, packages/perseus/src/widgets/__stories__/expression.stories.tsx, packages/perseus/src/widgets/__stories__/interactive-graph.stories.tsx, packages/perseus/src/widgets/__testdata__/interactive-graph.testdata.ts, packages/perseus/src/widgets/__tests__/passage.test.tsx, packages/perseus/src/widgets/interactive-graphs/interactive-graph-question-builder.ts, packages/perseus/src/widgets/interactive-graphs/mafs-graph.test.tsx, packages/perseus/src/widgets/interactive-graphs/mafs-graph.tsx, packages/perseus/src/widgets/interactive-graphs/stateful-mafs-graph.test.tsx, packages/perseus/src/widgets/interactive-graphs/stateful-mafs-graph.tsx

Don't want to be involved in this pull request? Comment #removeme and we won't notify you of further changes.

github-actions · 2024-08-08T19:50:22Z

npm Snapshot: Published

Good news!! We've packaged up the latest commit from this PR (d15fa13) and published it to npm. You
can install it using the tag PR1503.

Example:

yarn add @khanacademy/perseus@PR1503

If you are working in Khan Academy's webapp, you can run:

./dev/tools/bump_perseus_version.sh -t PR1503

github-actions · 2024-08-08T19:51:34Z

Size Change: +45 B (+0.01%)

Total Size: 855 kB

Filename	Size	Change
`packages/perseus/dist/es/index.js`	414 kB	+45 B (+0.01%)

ℹ️ View Unchanged

Filename	Size
`packages/kas/dist/es/index.js`	38.3 kB
`packages/kmath/dist/es/index.js`	4.27 kB
`packages/math-input/dist/es/index.js`	78.2 kB
`packages/math-input/dist/es/strings.js`	1.79 kB
`packages/perseus-core/dist/es/index.js`	1.48 kB
`packages/perseus-editor/dist/es/index.js`	276 kB
`packages/perseus-linter/dist/es/index.js`	21.8 kB
`packages/perseus/dist/es/strings.js`	3.29 kB
`packages/pure-markdown/dist/es/index.js`	3.67 kB
`packages/simple-markdown/dist/es/index.js`	12.4 kB

_{compressed-size-action}

Myranae · 2024-08-08T19:55:33Z

Haven't finished properly reviewing, but I believe some of this code is code I also changed in my bug fix PR. It's been approved, but I wanted to manually test before landing. Hoping to do that today, just ran into some local dev server / ZND issues. I'll totally update my PR if this one moves forward first though :)

benchristel · 2024-08-08T20:44:32Z

@Myranae I'll wait for you to land your PR. I need to test this on a ZND before landing it anyway.

Myranae · 2024-08-08T20:54:03Z

@benchristel It's landed :)

jeremywiebe

Cool. I was initially surprised by the size of the PR (number of files touched) but most of that is removing hintMode. I'm pretty happy we can use static mode and not introduce another new concept (assuming it all works out). Thanks Ben!

jeremywiebe · 2024-08-08T21:25:20Z

packages/perseus/src/widgets/interactive-graphs/mafs-graph.tsx

@@ -45,11 +45,11 @@ export type MafsGraphProps = {
    labels: InteractiveGraphProps["labels"];
    state: InteractiveGraphState;
    dispatch: React.Dispatch<InteractiveGraphAction>;
-    hintMode: boolean;
+    static: boolean | null | undefined;


I'm not sure if types from other uses of static force this, but can we make this static: boolean | undefined? I think there are many places that use this X | null | undefined pattern, but in reality, we rarely distinguish between null and undefined.

This is a nitpicky thing, but it's another place where the type is broader/more complicated for no reason.

The type boolean | null | undefined comes from WidgetProps, so I had to use the same type here.

I don't really feel comfortable changing WidgetProps, since it's so widely used (and it's one of our few defenses against writing widgets that can't handle production data).

jeremywiebe · 2024-08-08T21:27:10Z

packages/perseus/src/widgets/interactive-graphs/reducer/use-graph-config.ts

+    // The name isStatic (instead of static) avoids a collision with the
+    // `static` keyword in ES6. `static` is not a valid variable name, so
+    // naming this `isStatic` allows the object to be destructured without
+    // renaming the resulting variables.
+    isStatic: boolean;


Guess we'll want to avoid having a Perseus concept named "const" then, eh? 😂

codecov · 2024-08-08T23:18:30Z

Codecov Report

Attention: Patch coverage is 72.41379% with 8 lines in your changes missing coverage. Please review.

Project coverage is 70.61%. Comparing base (720e3bf) to head (d15fa13).

Files	Patch %	Lines
...widgets/interactive-graphs/stateful-mafs-graph.tsx	42.85%	8 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1503      +/-   ##
==========================================
+ Coverage   70.00%   70.61%   +0.61%     
==========================================
  Files         514      517       +3     
  Lines      106133   106174      +41     
  Branches     7629    10875    +3246     
==========================================
+ Hits        74296    74974     +678     
+ Misses      31720    31200     -520     
+ Partials      117        0     -117

Files	Coverage Δ
packages/perseus/src/hint-renderer.tsx	`90.96% <ø> (-0.06%)`	⬇️
...ackages/perseus/src/mixins/widget-prop-denylist.ts	`100.00% <ø> (ø)`
packages/perseus/src/renderer.tsx	`93.80% <100.00%> (+1.28%)`	⬆️
packages/perseus/src/widgets/interactive-graph.tsx	`51.05% <100.00%> (+0.14%)`	⬆️
...ctive-graphs/interactive-graph-question-builder.ts	`100.00% <100.00%> (ø)`
...seus/src/widgets/interactive-graphs/mafs-graph.tsx	`99.42% <100.00%> (ø)`
...widgets/interactive-graphs/stateful-mafs-graph.tsx	`88.96% <42.85%> (-4.98%)`	⬇️

... and 125 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 720e3bf...d15fa13. Read the comment docs.

Myranae · 2024-08-09T14:20:17Z

Doing some manual testing on this, I noticed the hints-renderer stories related to interactive graph hint mode don't work anymore because hint mode isn't a thing and graphs don't automatically become static when added as a hint. As a result, we may want to delete all the interactive graph stories from the Hints Renderer stories section, especially since we now have static graph stories instead. This story and all the interactive graph ones under it I mean.

benchristel · 2024-08-09T16:41:36Z

@jeremywiebe @Myranae I realized I missed a crucial part of this feature, which is displaying the correct answer instead of the start coords when the graph is in static mode. So I'll have to ask for another review.

I'd really like Nicole's eyes on this when she's well again, because I suspect she's already done the research to figure out how displaying the correct answer is supposed to work. I've got a solution, but I'm not sure it follows established patterns.

This reverts commit 83bebcf.

jeremywiebe

This is great. LGTM!

jeremywiebe · 2024-08-13T22:55:18Z

packages/perseus/src/__tests__/renderer.test.tsx

@@ -1352,30 +1352,6 @@ describe("renderer", () => {
            expect(el).not.toBeNull();
        });

-        it("should force the widget to be non-static if it has a problem number", () => {


Really glad we don't need to further commit to this hack. 😅

jeremywiebe · 2024-08-13T22:56:27Z

packages/perseus/src/widgets/__testdata__/interactive-graph.testdata.ts

+            .build();
+        result["widgets"] = {
+            ...result["widgets"],
+            "radio 1": {


Hrm... seems like we now we need a radioQuestionBuilder() 😃

jeremywiebe · 2024-08-13T22:57:58Z

packages/perseus/src/widgets/interactive-graphs/stateful-mafs-graph.tsx

@@ -128,5 +129,15 @@ export const StatefulMafsGraph = React.forwardRef<
        graph.startCoords,
    ]);

+    if (props.static) {


This becomes obvious fairly quickly as I read the code, but could you leave a comment here that if we're in static mode we display the correct answer? (unless you don't feel that's necessary)

benchristel self-assigned this Aug 8, 2024

benchristel requested review from jeremywiebe, nishasy and SonicScrewdriver August 8, 2024 19:47

khan-actions-bot requested a review from a team August 8, 2024 19:47

benchristel requested a review from Myranae August 8, 2024 19:52

jeremywiebe approved these changes Aug 8, 2024

View reviewed changes

benchristel force-pushed the benc/static-interactive-graphs branch from 3e33577 to 81db6a7 Compare August 8, 2024 23:11

benchristel force-pushed the benc/static-interactive-graphs branch from c3f51a8 to 02985f3 Compare August 9, 2024 20:20

benchristel and others added 9 commits August 13, 2024 14:49

Revert "Add hintMode to widget and Renderer props (#1431)"

869bdac

This reverts commit 83bebcf.

wip: add static mode to interactive graph

ec207ef

wip: changeset

3cd1afe

Turn off interactivity for static graphs

f6d7895

Put static property last

e1f670f

Display the correct answer when the graph is static

d658e00

Clarify test fixture names

ad23797

Fix types

ca3cd4d

Fix formatting

0f2821e

benchristel force-pushed the benc/static-interactive-graphs branch from 02985f3 to 0f2821e Compare August 13, 2024 21:50

benchristel added 2 commits August 13, 2024 14:53

Make static always mean static

e421cc1

Remove obsolete test

9cdb26e

benchristel requested a review from jeremywiebe August 13, 2024 22:31

jeremywiebe approved these changes Aug 13, 2024

View reviewed changes

Add comment

d15fa13

benchristel changed the title ~~Add "static" toggle to Interactive Graph widget~~ Add "static" toggle to Interactive Graph widget Aug 13, 2024

benchristel merged commit f083200 into main Aug 13, 2024
20 of 27 checks passed

benchristel deleted the benc/static-interactive-graphs branch August 13, 2024 23:33

khan-actions-bot mentioned this pull request Aug 13, 2024

Version Packages #1514

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add "static" toggle to Interactive Graph widget #1503

Add "static" toggle to Interactive Graph widget #1503

benchristel commented Aug 8, 2024 •

edited

Loading

khan-actions-bot commented Aug 8, 2024 •

edited

Loading

github-actions bot commented Aug 8, 2024 •

edited

Loading

github-actions bot commented Aug 8, 2024 •

edited

Loading

Myranae commented Aug 8, 2024

benchristel commented Aug 8, 2024

Myranae commented Aug 8, 2024

jeremywiebe left a comment

jeremywiebe Aug 8, 2024

benchristel Aug 9, 2024

jeremywiebe Aug 8, 2024

codecov bot commented Aug 8, 2024 •

edited

Loading

Myranae commented Aug 9, 2024 •

edited

Loading

benchristel commented Aug 9, 2024

jeremywiebe left a comment

jeremywiebe Aug 13, 2024

jeremywiebe Aug 13, 2024

jeremywiebe Aug 13, 2024

Add "static" toggle to Interactive Graph widget #1503

Add "static" toggle to Interactive Graph widget #1503

Conversation

benchristel commented Aug 8, 2024 • edited Loading

Summary:

Test plan:

khan-actions-bot commented Aug 8, 2024 • edited Loading

Gerald

github-actions bot commented Aug 8, 2024 • edited Loading

npm Snapshot: Published

github-actions bot commented Aug 8, 2024 • edited Loading

Myranae commented Aug 8, 2024

benchristel commented Aug 8, 2024

Myranae commented Aug 8, 2024

jeremywiebe left a comment

Choose a reason for hiding this comment

jeremywiebe Aug 8, 2024

Choose a reason for hiding this comment

benchristel Aug 9, 2024

Choose a reason for hiding this comment

jeremywiebe Aug 8, 2024

Choose a reason for hiding this comment

codecov bot commented Aug 8, 2024 • edited Loading

Codecov Report

Myranae commented Aug 9, 2024 • edited Loading

benchristel commented Aug 9, 2024

jeremywiebe left a comment

Choose a reason for hiding this comment

jeremywiebe Aug 13, 2024

Choose a reason for hiding this comment

jeremywiebe Aug 13, 2024

Choose a reason for hiding this comment

jeremywiebe Aug 13, 2024

Choose a reason for hiding this comment

benchristel commented Aug 8, 2024 •

edited

Loading

khan-actions-bot commented Aug 8, 2024 •

edited

Loading

github-actions bot commented Aug 8, 2024 •

edited

Loading

github-actions bot commented Aug 8, 2024 •

edited

Loading

codecov bot commented Aug 8, 2024 •

edited

Loading

Myranae commented Aug 9, 2024 •

edited

Loading