Ensure answerless options are assignable to NumberLine props #2424
Conversation
|
Size Change: 0 B Total Size: 466 kB ℹ️ View Unchanged
|
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (3b1d3ec) and published it to npm. You Example: pnpm add @khanacademy/perseus@PR2424If you are working in Khan Academy's webapp, you can run: ./dev/tools/bump_perseus_version.js -t PR2424 |
handeyeco
left a comment
handeyeco
left a comment
There was a problem hiding this comment.
Cool, fairly small PR and I feel like I learned a lot.
| describe.each([ | ||
| ["answerless", getAnswerlessItem("number-line", numberLineOptions)], | ||
| ["answerful", getAnswerfulItem("number-line", numberLineOptions)], | ||
| ])("given %s options", (_, {question}) => { | ||
| it("can be answered correctly", () => { |
There was a problem hiding this comment.
Nice, this is better than I was doing. I know you suggested to me but I didn't quite know how.
| const apiOptions: APIOptions = { | ||
| isMobile: false, | ||
| }; |
There was a problem hiding this comment.
Just curious, did you find that you needed this?
| const {renderer} = renderQuestion(question, apiOptions); | ||
|
|
||
| // Act | ||
| const [numberLine] = renderer.findWidgets("number-line 1"); |
There was a problem hiding this comment.
Did not know about findWidgets
| const [numberLine] = renderer.findWidgets("number-line 1"); | ||
| act(() => numberLine.movePosition(-2.5)); | ||
| const score = scorePerseusItemTesting( | ||
| question1, |
There was a problem hiding this comment.
I don't think you want to use question1 here
| const [numberLine] = renderer.findWidgets("number-line 1"); | ||
| act(() => numberLine.movePosition(3.5)); | ||
| const score = scorePerseusItemTesting( | ||
| question1, |
| ); | ||
|
|
||
| // Assert | ||
| expect(score).toHaveBeenAnsweredIncorrectly(); |
There was a problem hiding this comment.
Is there an invalid state we need to test? Or is it just correct/incorrect?
| type Props = ChangeableProps & { | ||
| range: [number, number]; | ||
| type Props = { | ||
| range: number[]; |
There was a problem hiding this comment.
I thought [number, number] was preferred when there was a fixed number of elements?
There was a problem hiding this comment.
The data-schema types have range: number[], and the parser (which is derived from data-schema) does too. Since we apparently don't guarantee that there will always be two elements, I opted to change this type to be compatible with data-schema rather than vice-versa.
| onChange: (arg1: any, arg2?: () => void | null | undefined) => void; | ||
| apiOptions: APIOptions; | ||
| keypadElement: HTMLElement | null | undefined; | ||
| keypadElement?: HTMLElement | undefined; |
There was a problem hiding this comment.
This couldn't be:
| keypadElement?: HTMLElement | undefined; | |
| keypadElement?: HTMLElement; |
|
@handeyeco turns out this change breaks the Storybook examples. I'm going to rework this. |
Summary:
This PR adds tests for the answerless case, and updates the types to reflect
what actually constitutes valid input.
The
numDivisionsprop is now required to be a number. The code forNumericInputis written in an (overly) clever way that treats 0 and null the same, but this is brittle and TypeScript doesn't like it. To clarify the code, we defaultnumDivisionsto 0 in the parser.Issue: LEMS-2976
Test plan:
TODO