Rough out UserInput type #1639
Rough out UserInput type #1639
Conversation
handeyeco
changed the title
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (dcb4699) and published it to npm. You Example: yarn add @khanacademy/perseus@PR1639If you are working in Khan Academy's webapp, you can run: ./dev/tools/bump_perseus_version.sh -t PR1639 |
|
Size Change: -156 B (-0.02%) Total Size: 862 kB
ℹ️ View Unchanged
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1639 +/- ##
==========================================
- Coverage 70.59% 70.49% -0.11%
==========================================
Files 561 583 +22
Lines 108194 112978 +4784
Branches 7959 11387 +3428
==========================================
+ Hits 76384 79639 +3255
- Misses 31688 33339 +1651
+ Partials 122 0 -122 see 1139 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
| @@ -149,7 +145,7 @@ export type Widget = { | |||
| // TODO(jeremy): I think this is actually a callback | |||
| focus?: () => unknown, | |||
| ) => void; | |||
| getUserInput?: () => WidgetUserInput | null | undefined; | |||
| getUserInput?: () => UserInput | null | undefined; | |||
There was a problem hiding this comment.
I have a hunch that my request will lead down a rabbit hole of fighting TypeScript, but would it be possible to make this Widget type generic so that the UserInput type would be a generic type? That way, the numeric-input could only legally return PerseusNumericInputUserInput.
If 5 min of trying doesn't work, we can push that out, but it's been a thought I've often had as I worked with this Widget type. (the same goes for return type of getSerializedState)
|
|
||
| type Props = WidgetProps<RenderProps, Rubric> & { | ||
| type Props = WidgetProps<RenderProps, PerseusCategorizerRubric> & { |
There was a problem hiding this comment.
thought: Originally, when Perseus lived in webapp, we built the types using a Perseus prefix. Today with Perseus being in its own repo and npm package, I feel like we could begin moving away from prefixing everything with Perseus. It's unrelated to this PR, but seeing all the PerseusXyz types brought it to my mind again.
| @@ -66,6 +66,7 @@ type Point = { | |||
| y: number; | |||
| }; | |||
|
|
|||
| // TODO: should this be using WidgetProps / PerseusLabelImageWidgetOptions? | |||
There was a problem hiding this comment.
Yes. Or... we use this approach for all widget RenderProps types where we explicitly define each key. I think it makes this more verbose but potentially clearer. @benchristel I think you've had thoughts about this before, haven't you?
Although not part of this PR, this should also be named RenderProps for consistency.
Summary:
I might have gone the wrong direction with this (as it is now).
I thought it would be beneficial to have widget Rubrics and UserInput in one place:
validation.types.ts. This way we have the information we need from the learner and the information we need to determine if the user input is correct/incorrect.However as I was going through this, I started to feel like we misnamed something:
WidgetProps<RenderProps, Rubric>. For some of these widgets, they were the same thing! For instance definition:RenderProps is the same as Rubric and then we just stick on another random prop (not to mention we have UserInput even though it's empty). On top of this, when we use
WidgetProps<RenderProps, Rubric>, we only useRubricforreviewModeRubricwhich is only used by a couple of components. Here's my guess at what's going on:timesandbuttonSetsto display the widget, but it only needsanswerFormsto grade the user input.WidgetPropshasreviewModeRubricas a legacy way of configuring or grading widgets. My guess is we need:WidgetProps<RenderProps>, removing/replacingreviewModeRubricRenderPropsshould be scoped to just the stuff it needs (per widget) to render, relying on the types from the widget's options when possibleWidgetOptionsper widget should continue to be focused on our external APIRubricper widget should be focused on just the stuff we need to grade the widgetUserInputshould be focused on just the things the learner sends for a widget to be gradedExpression is a good example:
The more I look at it, the more I think it's kind of a mess. Anyway this PR doesn't address any of this. But I was intentional about creating a specific
UserInputandRubrictype for most widgets even though they need further refinement.Issue: LEMS-2328
Test plan: