Add hintMode to widget and Renderer props #1431
Conversation
|
Size Change: -398 B (-0.05%) Total Size: 854 kB
ℹ️ View Unchanged
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1431 +/- ##
==========================================
+ Coverage 69.80% 70.61% +0.80%
==========================================
Files 505 507 +2
Lines 105660 105523 -137
Branches 7647 11448 +3801
==========================================
+ Hits 73761 74513 +752
+ Misses 31720 31010 -710
+ Partials 179 0 -179
... and 157 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
| @@ -23,6 +23,7 @@ const denylist = [ | |||
| "apiOptions", | |||
| "questionCompleted", | |||
| "findWidgets", | |||
| "hintMode", | |||
There was a problem hiding this comment.
Stuff like this makes me nervous that adding new props to widgets is not an entirely trivial process :/
There was a problem hiding this comment.
Question: How did you know this needed to get added here? Is that what made you nervous? That there are steps involved that we may not be fully aware of or that these steps even exist? I'm guessing the latter as I write this out, but could be both :)
There was a problem hiding this comment.
I searched the codebase for one of the other props in this list, with the goal of finding places where widget props are created. That led me to this file.
Tangent:
Looking more closely at how this list is used, I see:
perseus/packages/perseus/src/widgets/passage-ref.tsx
Lines 79 to 83 in 7bf32b7
This function is supposed to return a UserInput, so perhaps what we ought to be doing here is just creating an object that has the fields in UserInput. Then we could remove the denylist.
getUserInput: () => UserInput = () => ({
static: this.props.static,
reviewModeRubric: this.props.reviewModeRubric,
linterContext: this.props.linterContext,
isLastUsedWidget: this.props.isLastUsedWidget,
alignment: this.props.alignment,
});However, a quick search makes it seem like the return value of getUserInput is not actually used (it's passed to PassageRef.validate(), but that function doesn't do anything with it and just returns a hardcoded value). So I think maybe we can just delete all this code.
There was a problem hiding this comment.
Okay, getUserInput is used, by the Renderer (which calls it on each widget).
There was a problem hiding this comment.
😂 Got excited about potentially just deleting it all. It looks like the denylist is used in other places as well, but removeDenylistProps only seems to be used with the getUserInput here. Is the getUserInput used by Renderer this same method from PassageRef? In any case, probably still good to add hintMode here if it should be excluded when widgets serialize themselves. Thanks for looking into this!
There was a problem hiding this comment.
getUserInput is used as part of the scoring system. Each widget provides a simpleValidate() function that the Renderer calls. This function scores the widget in its current state.
simpleValidate typically calls the getUserInput instance function on itself. This function returns the input(s) that the user has provided (the selected radio option(s), the graph coordinate that the user has moved the point to, etc). The widget then calls a validate() static method on itself and passes the user input and the "rubric" (the widget options that define the correct answer) and returns the score (correct/incorrect).
The removeDenylistProps in this flow is a cheat and I'd love to get rid of it. Basically, it returns all widget props as the user input, except those that show up in the denylist.
Instead getUserInput on each widget should just return an object that represents the minimum necessary set of values (from props) that are needed by simpleValidate.
| @@ -31,6 +31,7 @@ export type StatefulMafsGraphProps = { | |||
| showTooltips: Required<InteractiveGraphProps["showTooltips"]>; | |||
| showProtractor: boolean; | |||
| labels: InteractiveGraphProps["labels"]; | |||
| hintMode: boolean; | |||
There was a problem hiding this comment.
Question: What determines if this should be added to InteractiveGraphProps like some of the other props here or just added as a boolean?
There was a problem hiding this comment.
InteractiveGraphProps["hintMode"] is equivalent to boolean, as far as the TypeScript compiler is concerned. But the meaning of boolean is immediately obvious to anyone who knows TypeScript, while to find out what InteractiveGraphProps["hintMode"] means, you have to go digging through two other files. You also have to know that hintMode is common to all widgets and not a special interactive-graph prop; otherwise, you're unlikely to look in the right place. I dislike having to dig through layers of type declarations to figure out what the actual type of something is, so I prefer to write types like boolean instead of InteractiveGraphProps["hintMode"].
There was a problem hiding this comment.
I always assumed it was done that way so that if the type of one changed it updated the type of the other. I think there are probably other ways of making sure to update it in all the right places though, such as tests and such, right?
There was a problem hiding this comment.
The type system / tsc itself will ensure that the types are kept consistent. That's the main benefit of a type system - keeping different things consistent 😄
There was a problem hiding this comment.
InteractiveGraphProps and all of the types defined in perseus-types.ts should not include props that the Renderer provides programmatically. The types in perseus-types.ts could be thought of as the data schema for Perseus (the serialized, at rest data format).
So Ben has it the right way here.
In the widget definitions, we've used the format InteractiveGraphProps["some_prop"] as a signal that the prop comes from the serialized widget options as opposed to something that is derived at runtime. All of these types/props could use a big cleanup/standardization, but it didn't get done during our Perseus Stabilization project and hasn't become a high enough priority yet.
I'd love to see a "gold standard" widget definition that included a convention for defining all the necessary types, file layout, test file location, etc. :) #someday.
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 (2161300) and published it to npm. You Example: yarn add @khanacademy/perseus@PR1431If you are working in Khan Academy's webapp, you can run: ./dev/tools/bump_perseus_version.sh -t PR1431 |
| @@ -23,6 +23,7 @@ const denylist = [ | |||
| "apiOptions", | |||
| "questionCompleted", | |||
| "findWidgets", | |||
| "hintMode", | |||
There was a problem hiding this comment.
getUserInput is used as part of the scoring system. Each widget provides a simpleValidate() function that the Renderer calls. This function scores the widget in its current state.
simpleValidate typically calls the getUserInput instance function on itself. This function returns the input(s) that the user has provided (the selected radio option(s), the graph coordinate that the user has moved the point to, etc). The widget then calls a validate() static method on itself and passes the user input and the "rubric" (the widget options that define the correct answer) and returns the score (correct/incorrect).
The removeDenylistProps in this flow is a cheat and I'd love to get rid of it. Basically, it returns all widget props as the user input, except those that show up in the denylist.
Instead getUserInput on each widget should just return an object that represents the minimum necessary set of values (from props) that are needed by simpleValidate.
| @@ -31,6 +31,7 @@ export type StatefulMafsGraphProps = { | |||
| showTooltips: Required<InteractiveGraphProps["showTooltips"]>; | |||
| showProtractor: boolean; | |||
| labels: InteractiveGraphProps["labels"]; | |||
| hintMode: boolean; | |||
There was a problem hiding this comment.
InteractiveGraphProps and all of the types defined in perseus-types.ts should not include props that the Renderer provides programmatically. The types in perseus-types.ts could be thought of as the data schema for Perseus (the serialized, at rest data format).
So Ben has it the right way here.
In the widget definitions, we've used the format InteractiveGraphProps["some_prop"] as a signal that the prop comes from the serialized widget options as opposed to something that is derived at runtime. All of these types/props could use a big cleanup/standardization, but it didn't get done during our Perseus Stabilization project and hasn't become a high enough priority yet.
I'd love to see a "gold standard" widget definition that included a convention for defining all the necessary types, file layout, test file location, etc. :) #someday.
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/[email protected] ### Major Changes - [#1435](#1435) [`364ccbecc`](364ccbe) Thanks [@handeyeco](https://github.com/handeyeco)! - Remove StatefulArticleEditor ### Minor Changes - [#1431](#1431) [`83bebcfaf`](83bebcf) Thanks [@benchristel](https://github.com/benchristel)! - Add a `hintMode` prop to `Renderer` and widgets. ### Patch Changes - Updated dependencies \[[`fedac0be5`](fedac0b), [`83bebcfaf`](83bebcf)]: - @khanacademy/[email protected] - @khanacademy/[email protected] ## @khanacademy/[email protected] ### Minor Changes - [#1431](#1431) [`83bebcfaf`](83bebcf) Thanks [@benchristel](https://github.com/benchristel)! - Add a `hintMode` prop to `Renderer` and widgets. ### Patch Changes - [#1424](#1424) [`fedac0be5`](fedac0b) Thanks [@catandthemachines](https://github.com/catandthemachines)! - Updating wonderblock-popover version and disable portal functionality in Expression Popover functionality. - Updated dependencies \[[`fedac0be5`](fedac0b)]: - @khanacademy/[email protected] ## @khanacademy/[email protected] ### Patch Changes - [#1424](#1424) [`fedac0be5`](fedac0b) Thanks [@catandthemachines](https://github.com/catandthemachines)! - Updating wonderblock-popover version and disable portal functionality in Expression Popover functionality. ## @khanacademy/[email protected] ### Patch Changes - Updated dependencies \[[`fedac0be5`](fedac0b)]: - @khanacademy/[email protected] Author: khan-actions-bot Reviewers: jeremywiebe Required Reviewers: Approved By: jeremywiebe Checks: ✅ codecov/project, ✅ codecov/patch, ✅ Upload Coverage (ubuntu-latest, 20.x), ⏭️ Publish npm snapshot, ✅ Jest Coverage (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Check builds for changes in size (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), ✅ gerald Pull Request URL: #1438
This reverts commit 83bebcf.
This reverts commit 83bebcf.
This reverts commit 83bebcf.
This reverts commit 83bebcf.
Summary:
Issue: XXX-XXXX
Test plan: