[InteractiveGraph] Interactive but unscored (implementation)#3466
[InteractiveGraph] Interactive but unscored (implementation)#3466
Conversation
🗄️ Schema Change: No Changes ✅ |
|
Size Change: +725 B (+0.14%) Total Size: 502 kB 📦 View Changed
ℹ️ View Unchanged
|
🛠️ Item Splitting: No Changes ✅ |
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (0cea41e) and published it to npm. You Example: pnpm add @khanacademy/perseus@PR3466If you are working in Khan Academy's frontend, you can run the below command. ./dev/tools/bump_perseus_version.ts -t PR3466If you are working in Khan Academy's webapp, you can run the below command. ./dev/tools/bump_perseus_version.js -t PR3466 |
| )} | ||
|
|
||
| {this.props.graph?.type && | ||
| shouldShowStartCoordsUI( |
There was a problem hiding this comment.
I don't think we want this to be hidden when graded: false, start position still seems valid to me.
There was a problem hiding this comment.
In fact we can't really hide "Correct answer" without a larger refactor. See: https://khanacademy.atlassian.net/browse/LEMS-3958?focusedCommentId=474872
… improve content editor ux
There was a problem hiding this comment.
@handeyeco added this component for the editor settings. This now contains all the toggle and drop down from the widget header to improve the UX. See slack discussion for details.
…G ungraded for now
|
@claude review once |
| static bestPractices = { | ||
| url: "https://www.khanacademy.org/internal-courses/content-creation-best-practices/xe46daa512cd9c644:question-writing/xe46daa512cd9c644:multiple-choice/a/stems", | ||
| label: "Interactive Graph best practices", | ||
| }; |
There was a problem hiding this comment.
🟡 The newly-added static bestPractices on InteractiveGraphEditor (interactive-graph-editor.tsx:165-168) is a copy-paste of the URL from RadioEditor.bestPractices — it ends in multiple-choice/a/stems and takes content authors to the Multiple-choice stems doc even though the link label reads "Interactive Graph best practices." Either replace the URL with a real Interactive Graph best-practices page or omit bestPractices here until that content exists.
Extended reasoning...
What the bug is. This PR introduces a new WidgetEditorSettings component that renders a "best practices" link using the optional static bestPractices property declared on each widget editor class (see packages/perseus-editor/src/components/widget-editor-settings.tsx and the Ed?.bestPractices read in widget-editor.tsx). As part of wiring this up, a bestPractices block was added to InteractiveGraphEditor at packages/perseus-editor/src/widgets/interactive-graph-editor/interactive-graph-editor.tsx:165-168:\n\nts\nstatic bestPractices = {\n url: "https://www.khanacademy.org/internal-courses/content-creation-best-practices/xe46daa512cd9c644:question-writing/xe46daa512cd9c644:multiple-choice/a/stems",\n label: "Interactive Graph best practices",\n};\n\n\nThe URL is byte-for-byte identical to the one on RadioEditor.bestPractices at packages/perseus-editor/src/widgets/radio/editor.tsx:46-49 (which is labeled "Multiple choice best practices"). Grep confirms these are the only two occurrences of this URL in the repo.\n\nThe code path that triggers it. When a content author opens the Interactive Graph widget in the editor, WidgetEditor.render reads Ed?.bestPractices and forwards it to WidgetEditorSettings, which renders <Link href={bestPractices.url} target="_blank">{bestPractices.label}</Link>. So the label "Interactive Graph best practices" is displayed, but clicking the link navigates to .../multiple-choice/a/stems — the Multiple-Choice stems documentation.\n\nWhy existing code doesn't prevent it. There is no validation tying label to url; each editor class declares the pair independently, and the WidgetEditorSettings component simply renders whatever strings it is given. The copy/paste happened before those checks would have had a chance to catch it.\n\nImpact. User-visible but not a functional failure. Content creators editing Interactive Graph questions will be sent to the wrong best-practices page. The link is still a valid Khan Academy resource, so nothing is broken, but the label/destination mismatch is misleading. Only editor users are affected — learners never see this link.\n\nHow to fix. Either (a) replace the URL with the actual Interactive Graph best-practices page, or (b) delete the static bestPractices block on InteractiveGraphEditor until that content exists. Option (b) is safest since WidgetEditorSettings already handles an absent bestPractices prop (see the bestPractices && (...) guard in widget-editor-settings.tsx). Either way, no other widget editor is impacted — only this one new class.\n\nStep-by-step proof.\n1. Apply this PR and open the exercise editor.\n2. Insert or focus an Interactive Graph widget; WidgetEditor mounts and reads Ed?.bestPractices where Ed = InteractiveGraphEditor.\n3. Because InteractiveGraphEditor.bestPractices is defined, WidgetEditorSettings renders a link: label = "Interactive Graph best practices", href = ".../multiple-choice/a/stems".\n4. Click the link → a new tab opens on the Multiple-Choice stems best-practices article, not an Interactive Graph one.\n5. Compare with packages/perseus-editor/src/widgets/radio/editor.tsx:46-49: identical URL, label "Multiple choice best practices". The label is correct there — which confirms the URL describes multiple-choice content and was copied verbatim into the new IG block without updating it.
There was a problem hiding this comment.
Currently the interactive graph best practices doesn't exist yet like the radio widget. But for now we can use this link for our widget fundamentals: https://khanacademy.atlassian.net/wiki/spaces/LC/pages/3295281289/Interactive+Graph+Widget#Adding-a-new-Interactive-Graph-Widget
Do you mind adding also a TODO comment to update the link once content team created it, see chat here
|
I made a dummy Frontend PR to spin up a ZND: https://github.com/Khan/frontend/pull/10557 |
There was a problem hiding this comment.
@handeyeco I added my review comments for the most part of the change request is changing the usage of spacing which is deprecated to sizing as documented here in WB
Also a note let's make sure that content team is aware of the expected behavior and this is align with what they're thinking. And lastly, when this PR will be deployed in frontend please double check that the failed test (if any) is not related to the changes, especially this PR is added the new props.
Please let me know if you have questions about my comments. 🙏🏼
| <BodyText id={labelId} tag="span"> | ||
| Alignment | ||
| </BodyText> | ||
| <Strut size={spacing.xSmall_8} /> |
There was a problem hiding this comment.
spacing is deprecated please use sizing instead
see: https://khan.github.io/wonder-blocks/?path=/docs/packages-tokens-sizing--docs#sizing-2
There was a problem hiding this comment.
IMO this shouldn't be a blocker, it's copying established patterns in other files.
There was a problem hiding this comment.
I would recommend to go with gap: sizing.size_080 (or size_060) in the View.style prop defined in line 38.
There was a problem hiding this comment.
re: established patterns
I've been doing a lot of migrations in both perseus and frontend to update our UI code to switch to newer patterns so we can use more modern CSS features instead of relying on previous patterns that were useful before. I'll take a note on migrating us from Strut/Spring to use CSS properties directly.
There was a problem hiding this comment.
@ivyolamit maybe it would be best to replace all uses of Strut in Perseus in a single (separate) PR.
There was a problem hiding this comment.
If moving to gap is not possible at this point, I'd recommend at least changing any of the Strut uses to have the same spacing size, that way there won't be flagged as a visual regression when this is converted to gap (which makes a bit harder to review in Chromatic).
There was a problem hiding this comment.
I just changed it to use gap (removing Strut)
| /> | ||
| ))} | ||
| </SingleSelect> | ||
| <Strut size={spacing.xxSmall_6} /> |
There was a problem hiding this comment.
See comment above about spacing is deprecated
There was a problem hiding this comment.
| return ( | ||
| <View | ||
| style={{ | ||
| paddingInline: spacing.small_12, |
There was a problem hiding this comment.
Please see spacing comment above.
There was a problem hiding this comment.
Ideally, the styles will be in a .css file so we can use classNames here. This is something Mark has been working in our code base since last year. But in the editor to my knowledge it's not fully migrated yet so we would need the !important tag.
I would recommend moving the styles in its css file, including other styles in this PR.
There was a problem hiding this comment.
It says it was added by you, do you mind submitting a fix?
There was a problem hiding this comment.
From the WB perspective, it is fine to keep the styles in an object for now. I'm about to start the CSS modules migration in Wonder Blocks, and one of my main goals is to continue supporting the style prop (instead of using className) so the migration can be done incrementally and it will be backwards compatible with our existing styling (cc @mark-fitzgerald for you to be aware of this).
| label: string; | ||
| }; | ||
|
|
||
| type WidgetEditorSettingsProps = { |
There was a problem hiding this comment.
I think this qualify as interface
something the team has discussed before on using type vs interface. And we would be in favor with using interface.
There was a problem hiding this comment.
It says it was added by you, do you mind submitting a fix?
| removePoint: "Remove Point", | ||
| graphKeyboardPrompt: "Press Shift + Enter to interact with the graph", | ||
| ungradedInteractiveGraph: | ||
| "Use this graph to check your thinking, but it does not count as your answer.", |
There was a problem hiding this comment.
Please don't forget to file the translations when deploying the frontend, it should add a comment in the PR on how to file it in a jira ticket.
|
TODO: Ivy and I decided to add the toggle switch in the editor behind a feature flag. |
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@77.4.0 ### Minor Changes - [#3466](#3466) [`9a1fd15302`](9a1fd15) Thanks [@handeyeco](https://github.com/handeyeco)! - Implement "ungraded" InteractiveGraphs: interactive IGs that aren't scored as part of the regular scoring flow ### Patch Changes - Updated dependencies \[[`9a1fd15302`](9a1fd15)]: - @khanacademy/perseus-score@8.8.0 - @khanacademy/perseus-core@26.0.1 - @khanacademy/keypad-context@3.2.46 - @khanacademy/kmath@2.4.4 - @khanacademy/math-input@26.4.18 - @khanacademy/perseus-linter@5.0.1 ## @khanacademy/perseus-editor@31.1.0 ### Minor Changes - [#3466](#3466) [`9a1fd15302`](9a1fd15) Thanks [@handeyeco](https://github.com/handeyeco)! - Implement "ungraded" InteractiveGraphs: interactive IGs that aren't scored as part of the regular scoring flow ### Patch Changes - Updated dependencies \[[`9a1fd15302`](9a1fd15)]: - @khanacademy/perseus@77.4.0 - @khanacademy/perseus-score@8.8.0 - @khanacademy/perseus-core@26.0.1 - @khanacademy/keypad-context@3.2.46 - @khanacademy/kmath@2.4.4 - @khanacademy/math-input@26.4.18 - @khanacademy/perseus-linter@5.0.1 ## @khanacademy/perseus-score@8.8.0 ### Minor Changes - [#3466](#3466) [`9a1fd15302`](9a1fd15) Thanks [@handeyeco](https://github.com/handeyeco)! - Implement "ungraded" InteractiveGraphs: interactive IGs that aren't scored as part of the regular scoring flow ### Patch Changes - Updated dependencies \[[`9a1fd15302`](9a1fd15)]: - @khanacademy/perseus-core@26.0.1 - @khanacademy/kmath@2.4.4 ## @khanacademy/keypad-context@3.2.46 ### Patch Changes - Updated dependencies \[[`9a1fd15302`](9a1fd15)]: - @khanacademy/perseus-core@26.0.1 ## @khanacademy/kmath@2.4.4 ### Patch Changes - Updated dependencies \[[`9a1fd15302`](9a1fd15)]: - @khanacademy/perseus-core@26.0.1 ## @khanacademy/math-input@26.4.18 ### Patch Changes - Updated dependencies \[[`9a1fd15302`](9a1fd15)]: - @khanacademy/perseus-core@26.0.1 - @khanacademy/keypad-context@3.2.46 ## @khanacademy/perseus-core@26.0.1 ### Patch Changes - [#3466](#3466) [`9a1fd15302`](9a1fd15) Thanks [@handeyeco](https://github.com/handeyeco)! - Implement "ungraded" InteractiveGraphs: interactive IGs that aren't scored as part of the regular scoring flow ## @khanacademy/perseus-linter@5.0.1 ### Patch Changes - Updated dependencies \[[`9a1fd15302`](9a1fd15)]: - @khanacademy/perseus-core@26.0.1 - @khanacademy/kmath@2.4.4 Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
## Summary: The problem: in #3466 we added a switch for `graded`. Everything was just dandy except that we broke publishes. That's because we started passing `graded` down to widgets (which we needed so IG could change its behavior based on the `graded` flag) but then every widgets started getting `graded`. Because of the janky way we serialize, that started adding `graded` in the WidgetOptions of some widgets ([ticket](https://khanacademy.atlassian.net/browse/LEMS-4105) and [Slack convo](https://khanacademy.slack.com/archives/C01AZ9H8TTQ/p1778089642003609)). Rather than discard unneeded fields, the publish pipeline just explodes when it sees a field it doesn't expect (which is totally cool and awesome) and it wasn't expecting `graded`. The best solution is to make data serialization opt-in vs opt-out (see [LEMS-4108](https://khanacademy.atlassian.net/browse/LEMS-4108)). However that's probably a medium risk change and we have a playtest today, so this change just skips around the denylist for `graded`. **Why this should be more safe than the last PR:** in #3466 there were some red flags...namely that I had to add `graded` to a bunch of tests. That was reverted as part of the bugfix (#3589) and the fact that I didn't need to add those changes back in this PR to pass checks indicated that this doesn't have the same problem as the last attempt. [LEMS-4108]: https://khanacademy.atlassian.net/browse/LEMS-4108?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ Author: handeyeco Reviewers: claude[bot], ivyolamit, catandthemachines Required Reviewers: Approved By: ivyolamit Checks: ⏭️ 1 check has been skipped, ✅ 10 checks were successful Pull Request URL: #3597

Summary:
InteractiveGraph: interactive but unscored. The goal is to give learners the ability to visualize math things while answering other parts of an exercise (like a NumericInput or a Radio). By leveraging the
gradedfield, we're able to take the IG out of the normal flow of scoring, but still leave it interactive (unlikestatic).The main bits of this were:
graded. Even thoughgradedexisted for a long time, it didn't seem to be used for anything except maybe IFrame; so we're using it for IG now.For the most part
gradedwas already kind of wired up: it existed in the data schema and the scoring function ignored widgets wheregraded === false. So this PR is more about adding some UI elements.Issue: LEMS-3970
Test plan:
Editor experience:
Screen.Recording.2026-04-24.at.3.12.09.PM.mov
Learner experience:
Screen.Recording.2026-04-24.at.3.13.11.PM.mov
In the ZND:
Screen.Recording.2026-04-27.at.2.55.15.PM.mov