Delete most InputNumber code, replace with NumericInput#3760
Delete most InputNumber code, replace with NumericInput#3760benchristel wants to merge 26 commits into
Conversation
…meric-input options type in data-schema
|
/review |
… code has been removed; widgets with type "input-number" will now render as NumericInput widgets. This involves a breaking change to the InputNumber widget types in data-schema. Callers should, as always, use the parser to migrate Perseus JSON to the latest schema version before using it, and avoid depending directly on the schema types.
|
Before this lands, we need to update the Go schema: https://github.com/Khan/webapp/pull/39496 |
…al function `scorePerseusItemWithInputNumberAsNumericInput`.
github-actions
Bot
added
item-splitting-change
schema-change
|
Size Change: -1.15 kB (-0.23%) Total Size: 507 kB 📦 View Changed
ℹ️ View Unchanged
|
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (2f62c39) and published it to npm. You Example: pnpm add @khanacademy/perseus@PR3760If you are working in Khan Academy's frontend, you can run the below command. ./dev/tools/bump_perseus_version.ts -t PR3760If you are working in Khan Academy's webapp, you can run the below command. ./dev/tools/bump_perseus_version.js -t PR3760 |
…ave the correct version
| message: "", | ||
| strict: true, | ||
| }, | ||
| ], |
There was a problem hiding this comment.
This test and the ones below could use the generator functions.
| ], | ||
| }, | ||
| alignment: "default", | ||
| } as InputNumberWidget, |
There was a problem hiding this comment.
We should not need to cast here.
| coefficient: false, | ||
| answers: [ | ||
| { | ||
| status: "correct", | ||
| value: 0.5, | ||
| maxError: 0, | ||
| simplify: "required", | ||
| answerForms: [], | ||
| message: "", | ||
| strict: true, | ||
| }, | ||
| ], |
There was a problem hiding this comment.
Could use the generator functions.
| message: "", | ||
| strict: true, | ||
| }, | ||
| ], |
There was a problem hiding this comment.
Could use the generator functions.
There was a problem hiding this comment.
This file doesn't have many usages. The data in it could be inlined into the tests that need it, and maybe replaced with generation function calls.
benchristel
added
schema-change-ack
SonicScrewdriver
left a comment
SonicScrewdriver
left a comment
There was a problem hiding this comment.
SO STOKED for this! It looks great :)
I just have one blocking comment to check in on the shift of the default of "strict" from false to true, as I want to make sure this was intentional. I also left a non-blocking comment about supporting Khanmigo.
| @@ -1,300 +1,7 @@ | |||
| import { | |||
| simplify: widgetData.simplify, | ||
| answerType: widgetData.answerType, | ||
| }, | ||
| label: widgetData.labelText, |
There was a problem hiding this comment.
I do wonder about removing simplify and answerType from Khanmigo, as that seems like helpful information for it to have in order to support student questions. I suspect VERY few of our Numeric Inputs have the labelText set, and obviously none of our our Input Numbers will have it either.
I can see that you've just brought this inline to match Numeric Input, but I wonder if we should check in with the Khanmigo team to see if we should be providing these values for both Input Number and Numeric Input.
|
|
||
| it("should consider decimals as the thousands separator in FR locale", () => { | ||
| // TODO(benchristel): This test seems wrong. The correct answer is 16.5, but | ||
| // 16.500,00 is accepted. |
There was a problem hiding this comment.
Ooooo that's a good find. I recall that the decimal separator logic is a little rough between languages, but I have a feeling we can fix this! :)
| simplify: inputNumberOptions.simplify, | ||
| message: "", | ||
| maxError: getMaxError(inputNumberOptions), | ||
| strict: true, |
There was a problem hiding this comment.
I thought we had strict set to false before, did you find something that indicated that it would be better to change it to true?
There was a problem hiding this comment.
It's been true from the beginning of the project. Here it is on main:
strict: false causes the "default" answer forms to be accepted in addition to the ones explicitly listed. Here's where that logic lives in the numeric-input scoring code:
// When an answer is set to strict, we validate using ONLY
// the provided answerForms. If strict is false, or if there
// were no provided answer forms, we will include all
// of the default answer forms in our validator.
if (!answer.strict || validatorForms.length === 0) {
validatorForms.push(...defaultAnswerForms);
}InputNumber never had any equivalent of strict — that is, it always used an explicit allowlist of answer forms. There was no way to also allow the "default" set of forms. Here's the original code for input-number answer forms:
There was a problem hiding this comment.
Oh weird! I was sure it was false. Okay wonderful! My apologies for the confusion
2 participants
Summary:
The input-number widget has been deprecated for... a decade? In this PR, we
replace the input-number editor and component with the numeric-input ones. To
make this a drop-in replacement, we migrate the input number data to a new v1
schema in the parser. The v1 schema is identical to the numeric-input schema.
Issue: LEMS-4112
Test plan:
Deploy the dev build to a ZND. Test that input-number widgets can be used and
scored. Test editing: existing input-numbers should display the numeric-input
editor (but the placeholder in the content will still say input-number).
scorePerseusItemWithInputNumberAsNumericInput