Create helper to build public widget options for Numeric Input #2174
Conversation
…t a widget export function to filter out rubric data from widget options for the Numeric Input widget
…options function and type
… of public widget functions
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (c7b55ef) and published it to npm. You Example: yarn add @khanacademy/perseus@PR2174If you are working in Khan Academy's webapp, you can run: ./dev/tools/bump_perseus_version.sh -t PR2174 |
|
Size Change: +70 B (0%) Total Size: 1.48 MB
ℹ️ View Unchanged
|
Myranae
requested review from
jeremywiebe,
handeyeco and
benchristel
and removed request for
jeremywiebe and
handeyeco
| @@ -544,6 +545,7 @@ export type WidgetScorerFunction = ( | |||
| * A union type of all the functions that provide public widget options. | |||
| */ | |||
| export type PublicWidgetOptionsFunction = | |||
| | typeof getNumericInputPublicWidgetOptions | |||
There was a problem hiding this comment.
Is this file the best place for this union? It is used to type the getPublicWidgetOptionsFunction in the widgets.ts file, which was created to be the function used in the editor to access the correct getPublicWidgetOptions function.
| const {answers: _, answerForms: __, ...publicWidgetOptions} = options; | ||
| return publicWidgetOptions; |
There was a problem hiding this comment.
Is there any reason this would be preferable or not preferable to building out the specific object we're looking for like I was doing previously? ex. {size: options.size, static: options.static, etc.}
I could see this being less brittle as it makes it easier to add fields to the widgetOptions and have them be automatically included. If this is preferable, should I go through and update the functions that do this the previous way?
There was a problem hiding this comment.
I don't think it matters too much one way or the other. The tests ensure that we include the correct fields either way.
I guess one way of approaching it might be to ask, "when we add fields to the widget options, are they more likely to be public or private?" If the code is written the way you have it here, we won't need to change it when adding public options, but we will need to change it when adding private options. On the other hand, if we explicitly list the public fields, we won't need to change this code when adding private options, but we will need to change it when adding public options.
Then there's the question of what's easier to read. The current approach has the advantage of being short, but it might be a bit obscure.
Ultimately, it's a judgment call, and a two-way door, so we can just pick an approach and, worst case, change it later.
| type NumericInputPublicWidgetOptions = { | ||
| labelText?: PerseusNumericInputWidgetOptions["labelText"]; | ||
| size: PerseusNumericInputWidgetOptions["size"]; | ||
| coefficient: PerseusNumericInputWidgetOptions["coefficient"]; | ||
| rightAlign?: PerseusNumericInputWidgetOptions["rightAlign"]; | ||
| static: PerseusNumericInputWidgetOptions["static"]; |
There was a problem hiding this comment.
I left out both answers and answerForms as they both appear to contain information related to the correct answer. 'answerForms` even has a note that it's supposedly used in examples, but might not even be used anymore.
There was a problem hiding this comment.
I think answerForms should actually be considered public information. See e.g. the examples() method on NumericInput, which apparently generates learner-facing instructions based on the answerForms that are accepted.
| type NumericInputPublicWidgetOptions = { | ||
| labelText?: PerseusNumericInputWidgetOptions["labelText"]; | ||
| size: PerseusNumericInputWidgetOptions["size"]; | ||
| coefficient: PerseusNumericInputWidgetOptions["coefficient"]; | ||
| rightAlign?: PerseusNumericInputWidgetOptions["rightAlign"]; | ||
| static: PerseusNumericInputWidgetOptions["static"]; |
There was a problem hiding this comment.
I think answerForms should actually be considered public information. See e.g. the examples() method on NumericInput, which apparently generates learner-facing instructions based on the answerForms that are accepted.
| const {answers: _, answerForms: __, ...publicWidgetOptions} = options; | ||
| return publicWidgetOptions; |
There was a problem hiding this comment.
I don't think it matters too much one way or the other. The tests ensure that we include the correct fields either way.
I guess one way of approaching it might be to ask, "when we add fields to the widget options, are they more likely to be public or private?" If the code is written the way you have it here, we won't need to change it when adding public options, but we will need to change it when adding private options. On the other hand, if we explicitly list the public fields, we won't need to change this code when adding private options, but we will need to change it when adding public options.
Then there's the question of what's easier to read. The current approach has the advantage of being short, but it might be a bit obscure.
Ultimately, it's a judgment call, and a two-way door, so we can just pick an approach and, worst case, change it later.
| @@ -544,6 +545,7 @@ export type WidgetScorerFunction = ( | |||
| * A union type of all the functions that provide public widget options. | |||
| */ | |||
| export type PublicWidgetOptionsFunction = | |||
| | typeof getNumericInputPublicWidgetOptions | |||
Summary:
Adds a function that takes a Numeric Input widget's full widget options and removes correct answer information. It also adds the function to the widget's widget export and adds a test confirming the function works as expected.
Issue: LEMS-2767
Test plan: