Refactor the cs-program widget to use a getUserInput function #1697
Conversation
|
Size Change: +4 B (0%) Total Size: 864 kB
ℹ️ View Unchanged
|
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (6dea81a) and published it to npm. You Example: yarn add @khanacademy/perseus@PR1697If you are working in Khan Academy's webapp, you can run: ./dev/tools/bump_perseus_version.sh -t PR1697 |
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
| getUserInput(): PerseusCSProgramUserInput { | ||
| return CSProgram.getUserInputFromProps(this.props); | ||
| } |
There was a problem hiding this comment.
I wonder if we need the getUserInputFromProps method?
| getUserInput(): PerseusCSProgramUserInput { | |
| return CSProgram.getUserInputFromProps(this.props); | |
| } | |
| getUserInput(): PerseusCSProgramUserInput { | |
| return { | |
| status: this.props.status, | |
| message: this.props.message, | |
| }; | |
| } |
There was a problem hiding this comment.
Ooo, for sure. I think I was just following the framework for other widgets that use getUserInput; it seems like most of them use this method. Maybe leave this for now and have a ticket to go through and refactor out getUserInputFromProps from as many widgets as possible? I'm not sure I understand why they were there in the first place. Looks like the method might be used in tests 🤔
There was a problem hiding this comment.
Okay, so first I kept the method and set up a test with it. I was following the example from categorizer's test. But then I tried to use the example from group's test and realized that cs-program uses an iframe. Checking out iframe, it doesn't have a getUserInputFromProps method, and it does it the way you suggest here. So I just did it this way in the end XD Not sure how to add a test though since iframe doesn't have any tests for getUserInput either. Do you have any suggestions @handeyeco ? (Also, I just picked up the ticket to split out validation logic from iframe, so we'll see if doing that first gives ideas here)
There was a problem hiding this comment.
Yeah, I don't really know. Maybe something like:
// Arrange
const widgetRef: CSProgram | null = null;
render(<CSProgram ref={(ref) => widgetRef = ref} status="test status" message="test message />);
// Act
const result = widgetRef.getUserInput();
// Assert
expect(result.status).toBe("test status");
expect(result.message).toBe("test message");I wouldn't lose sleep on it. We'll probably need to add tests to all the widgets for this and this could be a part of that process.
|
Also I wonder if it'd be worth adding a test for this. |
| it("should show default user input before user interaction", () => { | ||
| const apiOptions = { | ||
| isMobile: false, | ||
| } as const; | ||
|
|
||
| const {renderer} = renderQuestion(question1, apiOptions); | ||
| const userInput = | ||
| renderer.getUserInput()[0] as PerseusCSProgramUserInput; | ||
|
|
||
| expect(userInput.status).toBe("incomplete"); | ||
| expect(userInput.message).toBe(null); | ||
| }); | ||
|
|
||
| it("should snapshot default user input before user interaction", () => { | ||
| // Arrange | ||
| const {renderer} = renderQuestion(question1); | ||
|
|
||
| // Act | ||
| const userInput = renderer.getUserInput(); | ||
|
|
||
| // Assert | ||
| expect(userInput).toMatchInlineSnapshot(` | ||
| [ | ||
| { | ||
| "message": null, | ||
| "status": "incomplete", | ||
| }, | ||
| ] | ||
| `); | ||
| }); |
There was a problem hiding this comment.
@handeyeco So I added two different versions of what seem to be possibly the same test. Do either of these seem valuable? I wasn't able to figure out how to get an updated message from the renderer, but we can test the default at least.
There was a problem hiding this comment.
I think the first one is useful. Thanks!
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 - [#1696](#1696) [`3e1697229`](3e16972) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Remove deprecated `Util.widgetShouldHighlight` function ### Minor Changes - [#1696](#1696) [`3e1697229`](3e16972) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Migrate `Widget` type to be an TypeScript `interface` so each widget can declare it implements it explicitly. ### Patch Changes - [#1697](#1697) [`b9d84ccba`](b9d84cc) Thanks [@Myranae](https://github.com/Myranae)! - Refactor cs-program to use a getUserInputFunction - [#1713](#1713) [`bcd32425c`](bcd3242) Thanks [@handeyeco](https://github.com/handeyeco)! - Some minor cleanup related to validators - [#1708](#1708) [`8e95e00c4`](8e95e00) Thanks [@Myranae](https://github.com/Myranae)! - Move validation logic out of the orderer widget and add tests ## @khanacademy/[email protected] ### Patch Changes - Updated dependencies \[[`b9d84ccba`](b9d84cc), [`bcd32425c`](bcd3242), [`8e95e00c4`](8e95e00), [`3e1697229`](3e16972), [`3e1697229`](3e16972)]: - @khanacademy/[email protected] Author: khan-actions-bot Reviewers: jeremywiebe Required Reviewers: Approved By: jeremywiebe Checks: ⏭️ Publish npm snapshot, ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ gerald Pull Request URL: #1714
Summary:
The cs-program widget previously did not have a
getUserInputfunction. This function is necessary as we split the scoring logic out of the renderer system, so this adds and integrates the function.Issue: LEMS-2459
Test plan: