Create the foundation for a new save warnings utility function. Not ready for use yet. #2969
Conversation
… warnings utility function. Not ready for use yet.
🗄️ Schema Change: No Changes ✅ |
github-actions
bot
added
item-splitting-change
and removed
item-splitting-change
labels
🛠️ Item Splitting: No Changes ✅ |
|
Size Change: +656 B (+0.13%) Total Size: 498 kB
ℹ️ View Unchanged
|
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (6bdc6f2) and published it to npm. You Example: pnpm add @khanacademy/perseus@PR2969If you are working in Khan Academy's frontend, you can run the below command. ./dev/tools/bump_perseus_version.ts -t PR2969If you are working in Khan Academy's webapp, you can run the below command. ./dev/tools/bump_perseus_version.js -t PR2969 |
nishasy
requested review from
SonicScrewdriver,
anakaren-rojas,
ivyolamit and
mark-fitzgerald
as code owners
|
|
||
| // Only check widgets that are actually referenced in the content | ||
| // (widgets can remain in the widgets object after being deleted from content) | ||
| for (const [widgetId, widget] of Object.entries(widgets)) { |
There was a problem hiding this comment.
I think we should use the traverse() helper function to go through an item, because we could have complex widgets (like graded-group) which contain other widgets. I think we want to validate widgets inside those also, right?
See:
| // Only check widgets that are actually referenced in the content | ||
| // (widgets can remain in the widgets object after being deleted from content) | ||
| for (const [widgetId, widget] of Object.entries(widgets)) { | ||
| if (!content.includes(widgetId)) { |
There was a problem hiding this comment.
For safety sake, let's use the addWidget() (poorly named as it simply builds a widget placeholder string) when checking if the widget ID is in the content.
There was a problem hiding this comment.
If my other PR is valid, then I don't think this will be a concern anymore.
| switch (widget.type) { | ||
| case "radio": |
There was a problem hiding this comment.
Could we just create a dictionary of widget type to validator function here? I kinda feel like the pattern we should use is to create a new function on the WidgetLogic type and then look it up the way we do the various options defined there (such as getCurrentVersion() does).
| - Radio (already done) | ||
| */ | ||
|
|
||
| function getSaveWarningsForRadioWidget(widget: RadioWidget): Array<string> { |
There was a problem hiding this comment.
I think it'd make sense to co-locate this with each widget's logic in perseus-core. What do you think?
| React.useEffect(() => { | ||
| const warnings = getSaveWarningsForItem({ | ||
| question: question ?? {content: "", widgets: {}, images: {}}, | ||
| hints: [...(hints ?? [])], |
There was a problem hiding this comment.
Why do we need to spread the result here? If there's a reason, can you adda comment why? Tbh, if I saw this, I'd try to change it to hints: hints ?? [],
| let hasCorrectChoice = false; | ||
| for (const choice of widget.options.choices) { | ||
| if (choice.correct) { | ||
| hasCorrectChoice = true; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
suggestion: You could use a higher-order function from the Array type to shorten this. Whether it's clearer, I'll leave up to you! 😁
| let hasCorrectChoice = false; | |
| for (const choice of widget.options.choices) { | |
| if (choice.correct) { | |
| hasCorrectChoice = true; | |
| break; | |
| } | |
| } | |
| const hasCorrectChoice = widget.options.choices.filter(c => c.correct).length > 0; |
There was a problem hiding this comment.
Looks like I can also use some
Summary:
(Proof of concept)
Foundation for a new utility that can be called to check a PerseusItem for all save warnings.
The intention is to evetually remove all the
ref.getSaveWarnings()everywhere.Issue: https://khanacademy.atlassian.net/browse/LEMS-3643
Test plan:
pnpm jest packages/perseus/src/util/get-save-warnings-for-item.test.tsStorybook
/?path=/story/editors-editorpage--with-save-warnings-new-utilProof it's working
Screen.Recording.2025-10-16.at.6.21.29.PM.mov