-
Notifications
You must be signed in to change notification settings - Fork 364
Convert Expression to functional component and improve testing and typing #3157
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Reuse actual DOM target instead of dynamically-assigned textarea ID, ensuring focus() returns a truthful result even before the ID is set or when focus was already correct.
To cover focus() behavior when the input lacks an ID and to verify insert() works immediately after mount without prior focus.
…s PR Make more type safe by referencing Widget type
…anuary-2026' into tb/LEMS-3804/expression-conversion-2
…ssion to functional component and improve testing and typing
🗄️ Schema Change: No Changes ✅ |
|
Size Change: +256 B (+0.05%) Total Size: 487 kB
ℹ️ View Unchanged
|
🛠️ Item Splitting: No Changes ✅ |
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (f887a1a) and published it to npm. You Example: pnpm add @khanacademy/perseus@PR3157If you are working in Khan Academy's frontend, you can run the below command. ./dev/tools/bump_perseus_version.ts -t PR3157If you are working in Khan Academy's webapp, you can run the below command. ./dev/tools/bump_perseus_version.js -t PR3157 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small TS fix for something I missed in the previous conversion PR
Change one to be wrapped by renderer, remove unneeded story toggling answer visibility, remove answerless story
…s to be descriptive
|
|
||
| /** This story shows how the expression widget looks when the keypad is | ||
| * configured with _every_ option it supports. */ | ||
| // TODO: use a Renderer wrapper rather than rendering this directly | ||
| export const ShowAnswerButton = (): React.ReactElement => { | ||
| const [showAnswer, setShowAnswer] = React.useState(true); | ||
| return ( | ||
| <div style={{padding: "2rem"}}> | ||
| <label> | ||
| <input | ||
| type="checkbox" | ||
| checked={showAnswer} | ||
| onChange={() => setShowAnswer(!showAnswer)} | ||
| /> | ||
| Show answer | ||
| </label> | ||
| <ServerItemRendererWithDebugUI | ||
| item={showAnswer ? expressionItem4Static : expressionItem4} | ||
| /> | ||
| </div> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment had been updated incorrectly. This was for showing static using a checkbox. Since we now have a static story (which can change back and forth using the Perseus JSON if needed), I got rid of this one.
| }), | ||
| ); | ||
|
|
||
| export const expressionItemKitchenSink: PerseusItem = createExpressionItemJson( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually a new testdata item, but for some reason it's being compared to the static data item in the PR. Only the name of the static data item changed and this one is just added after it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a number of new tests that Claude recommended. Not sure if all of them are useful, though they sounded useful at the time. Let me know if any are unnecessary and just take up time. I think most are the cover the new KeypadInputWithInterface wrapper functionality.
… disabled stories The story called for a button that wasn't there in the keypad, so just had to add the right buttons to the test data
anakaren-rojas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work on the conversion!
I noticed this uses a nested wrapper pattern that differs from our other recently-converted widgets, which call useDependencies() directly.
Was there a specific reason for the extra wrapper? If not, for consistency sake, can you simplify to match the pattern used in the modernized widgets?
It's okay and probably wanted for there to be an editor error for this widget in the associated stories
Thanks for pointing this out, @anakaren-rojas! I believe I've updated this to match other functional components now (doesn't use the wrapper for analytics, uses |
anakaren-rojas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What an undertaking! Great job!
jeremywiebe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very cool!! Thanks for switching this over! One less class!!!
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/perseus@74.0.0 ### Major Changes - [#3192](#3192) [`2d57e51583`](2d57e51) Thanks [@benchristel](https://github.com/benchristel)! - Remove the deprecated `KEScore` type. - [#3188](#3188) [`ea593fef51`](ea593fe) Thanks [@benchristel](https://github.com/benchristel)! - Remove the deprecated `getUserInput` method from `Renderer`, and delete the `UserInputArray` type. - [#3189](#3189) [`3f6e33047d`](3f6e330) Thanks [@benchristel](https://github.com/benchristel)! - Remove the deprecated `getUserInputLegacy()` method of `ServerItemRenderer`. Callers should use `getUserInput()` instead. ### Minor Changes - [#3186](#3186) [`4fc881daf8`](4fc881d) Thanks [@ivyolamit](https://github.com/ivyolamit)! - [Radio] Remove getCorrectUserInput from the radio widget to hide the "Static" switch in the widget editor - [#3157](#3157) [`9cba5197a2`](9cba519) Thanks [@Myranae](https://github.com/Myranae)! - Convert Expression to functional component and improve testing and typing - [#3178](#3178) [`5756410e90`](5756410) Thanks [@mahtabsabet](https://github.com/mahtabsabet)! - Fix JIPT localization for graphie labels ### Patch Changes - [#3193](#3193) [`d5b27f0aad`](d5b27f0) Thanks [@Myranae](https://github.com/Myranae)! - Fix console errors from adding and removing radio choices - Updated dependencies \[[`2d57e51583`](2d57e51), [`e34c3db786`](e34c3db), [`29c3c697fb`](29c3c69), [`ea593fef51`](ea593fe), [`9cba5197a2`](9cba519), [`779c55b0c3`](779c55b)]: - @khanacademy/perseus-core@23.0.0 - @khanacademy/math-input@26.3.10 - @khanacademy/perseus-linter@4.6.12 - @khanacademy/keypad-context@3.2.30 - @khanacademy/kmath@2.2.30 - @khanacademy/perseus-score@8.2.6 ## @khanacademy/perseus-core@23.0.0 ### Major Changes - [#3192](#3192) [`2d57e51583`](2d57e51) Thanks [@benchristel](https://github.com/benchristel)! - Remove the deprecated `KEScore` type. - [#3188](#3188) [`ea593fef51`](ea593fe) Thanks [@benchristel](https://github.com/benchristel)! - Remove the deprecated `getUserInput` method from `Renderer`, and delete the `UserInputArray` type. ### Minor Changes - [#3157](#3157) [`9cba5197a2`](9cba519) Thanks [@Myranae](https://github.com/Myranae)! - Convert Expression to functional component and improve testing and typing ## @khanacademy/perseus-editor@28.10.0 ### Minor Changes - [#3186](#3186) [`4fc881daf8`](4fc881d) Thanks [@ivyolamit](https://github.com/ivyolamit)! - [Radio] Remove getCorrectUserInput from the radio widget to hide the "Static" switch in the widget editor ### Patch Changes - Updated dependencies \[[`d5b27f0aad`](d5b27f0), [`4fc881daf8`](4fc881d), [`2d57e51583`](2d57e51), [`e34c3db786`](e34c3db), [`29c3c697fb`](29c3c69), [`ea593fef51`](ea593fe), [`9cba5197a2`](9cba519), [`3f6e33047d`](3f6e330), [`5756410e90`](5756410), [`779c55b0c3`](779c55b)]: - @khanacademy/perseus@74.0.0 - @khanacademy/perseus-core@23.0.0 - @khanacademy/math-input@26.3.10 - @khanacademy/perseus-linter@4.6.12 - @khanacademy/keypad-context@3.2.30 - @khanacademy/kmath@2.2.30 - @khanacademy/perseus-score@8.2.6 ## @khanacademy/keypad-context@3.2.30 ### Patch Changes - Updated dependencies \[[`2d57e51583`](2d57e51), [`ea593fef51`](ea593fe), [`9cba5197a2`](9cba519)]: - @khanacademy/perseus-core@23.0.0 ## @khanacademy/kmath@2.2.30 ### Patch Changes - Updated dependencies \[[`2d57e51583`](2d57e51), [`ea593fef51`](ea593fe), [`9cba5197a2`](9cba519)]: - @khanacademy/perseus-core@23.0.0 ## @khanacademy/math-input@26.3.10 ### Patch Changes - [#3190](#3190) [`e34c3db786`](e34c3db) Thanks [@mark-fitzgerald](https://github.com/mark-fitzgerald)! - Update Storybook to v9.1.17 (security patch) - Updated dependencies \[[`2d57e51583`](2d57e51), [`ea593fef51`](ea593fe), [`9cba5197a2`](9cba519)]: - @khanacademy/perseus-core@23.0.0 - @khanacademy/keypad-context@3.2.30 ## @khanacademy/perseus-linter@4.6.12 ### Patch Changes - [#2921](#2921) [`29c3c697fb`](29c3c69) Thanks [@nishasy](https://github.com/nishasy)! - [Image] | (CX) | Add issues panel warning for long alt text - [#3183](#3183) [`779c55b0c3`](779c55b) Thanks [@anakaren-rojas](https://github.com/anakaren-rojas)! - adds linter to radio editor - triggered when NOTA and another choice are marked as correct - Updated dependencies \[[`2d57e51583`](2d57e51), [`ea593fef51`](ea593fe), [`9cba5197a2`](9cba519)]: - @khanacademy/perseus-core@23.0.0 - @khanacademy/kmath@2.2.30 ## @khanacademy/perseus-score@8.2.6 ### Patch Changes - Updated dependencies \[[`2d57e51583`](2d57e51), [`ea593fef51`](ea593fe), [`9cba5197a2`](9cba519)]: - @khanacademy/perseus-core@23.0.0 - @khanacademy/kmath@2.2.30 Author: khan-actions-bot Reviewers: benchristel Required Reviewers: Approved By: benchristel Checks: ⏭️ 2 checks have been skipped, ✅ 6 checks were successful Pull Request URL: #3187
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/perseus@74.0.0 ### Major Changes - [#3192](#3192) [`2d57e51583`](2d57e51) Thanks [@benchristel](https://github.com/benchristel)! - Remove the deprecated `KEScore` type. - [#3188](#3188) [`ea593fef51`](ea593fe) Thanks [@benchristel](https://github.com/benchristel)! - Remove the deprecated `getUserInput` method from `Renderer`, and delete the `UserInputArray` type. - [#3189](#3189) [`3f6e33047d`](3f6e330) Thanks [@benchristel](https://github.com/benchristel)! - Remove the deprecated `getUserInputLegacy()` method of `ServerItemRenderer`. Callers should use `getUserInput()` instead. ### Minor Changes - [#3186](#3186) [`4fc881daf8`](4fc881d) Thanks [@ivyolamit](https://github.com/ivyolamit)! - [Radio] Remove getCorrectUserInput from the radio widget to hide the "Static" switch in the widget editor - [#3157](#3157) [`9cba5197a2`](9cba519) Thanks [@Myranae](https://github.com/Myranae)! - Convert Expression to functional component and improve testing and typing - [#3178](#3178) [`5756410e90`](5756410) Thanks [@mahtabsabet](https://github.com/mahtabsabet)! - Fix JIPT localization for graphie labels ### Patch Changes - [#3193](#3193) [`d5b27f0aad`](d5b27f0) Thanks [@Myranae](https://github.com/Myranae)! - Fix console errors from adding and removing radio choices - Updated dependencies \[[`2d57e51583`](2d57e51), [`e34c3db786`](e34c3db), [`29c3c697fb`](29c3c69), [`ea593fef51`](ea593fe), [`9cba5197a2`](9cba519), [`779c55b0c3`](779c55b)]: - @khanacademy/perseus-core@23.0.0 - @khanacademy/math-input@26.3.10 - @khanacademy/perseus-linter@4.6.12 - @khanacademy/keypad-context@3.2.30 - @khanacademy/kmath@2.2.30 - @khanacademy/perseus-score@8.2.6 ## @khanacademy/perseus-core@23.0.0 ### Major Changes - [#3192](#3192) [`2d57e51583`](2d57e51) Thanks [@benchristel](https://github.com/benchristel)! - Remove the deprecated `KEScore` type. - [#3188](#3188) [`ea593fef51`](ea593fe) Thanks [@benchristel](https://github.com/benchristel)! - Remove the deprecated `getUserInput` method from `Renderer`, and delete the `UserInputArray` type. ### Minor Changes - [#3157](#3157) [`9cba5197a2`](9cba519) Thanks [@Myranae](https://github.com/Myranae)! - Convert Expression to functional component and improve testing and typing ## @khanacademy/perseus-editor@28.10.0 ### Minor Changes - [#3186](#3186) [`4fc881daf8`](4fc881d) Thanks [@ivyolamit](https://github.com/ivyolamit)! - [Radio] Remove getCorrectUserInput from the radio widget to hide the "Static" switch in the widget editor ### Patch Changes - Updated dependencies \[[`d5b27f0aad`](d5b27f0), [`4fc881daf8`](4fc881d), [`2d57e51583`](2d57e51), [`e34c3db786`](e34c3db), [`29c3c697fb`](29c3c69), [`ea593fef51`](ea593fe), [`9cba5197a2`](9cba519), [`3f6e33047d`](3f6e330), [`5756410e90`](5756410), [`779c55b0c3`](779c55b)]: - @khanacademy/perseus@74.0.0 - @khanacademy/perseus-core@23.0.0 - @khanacademy/math-input@26.3.10 - @khanacademy/perseus-linter@4.6.12 - @khanacademy/keypad-context@3.2.30 - @khanacademy/kmath@2.2.30 - @khanacademy/perseus-score@8.2.6 ## @khanacademy/keypad-context@3.2.30 ### Patch Changes - Updated dependencies \[[`2d57e51583`](2d57e51), [`ea593fef51`](ea593fe), [`9cba5197a2`](9cba519)]: - @khanacademy/perseus-core@23.0.0 ## @khanacademy/kmath@2.2.30 ### Patch Changes - Updated dependencies \[[`2d57e51583`](2d57e51), [`ea593fef51`](ea593fe), [`9cba5197a2`](9cba519)]: - @khanacademy/perseus-core@23.0.0 ## @khanacademy/math-input@26.3.10 ### Patch Changes - [#3190](#3190) [`e34c3db786`](e34c3db) Thanks [@mark-fitzgerald](https://github.com/mark-fitzgerald)! - Update Storybook to v9.1.17 (security patch) - Updated dependencies \[[`2d57e51583`](2d57e51), [`ea593fef51`](ea593fe), [`9cba5197a2`](9cba519)]: - @khanacademy/perseus-core@23.0.0 - @khanacademy/keypad-context@3.2.30 ## @khanacademy/perseus-linter@4.6.12 ### Patch Changes - [#2921](#2921) [`29c3c697fb`](29c3c69) Thanks [@nishasy](https://github.com/nishasy)! - [Image] | (CX) | Add issues panel warning for long alt text - [#3183](#3183) [`779c55b0c3`](779c55b) Thanks [@anakaren-rojas](https://github.com/anakaren-rojas)! - adds linter to radio editor - triggered when NOTA and another choice are marked as correct - Updated dependencies \[[`2d57e51583`](2d57e51), [`ea593fef51`](ea593fe), [`9cba5197a2`](9cba519)]: - @khanacademy/perseus-core@23.0.0 - @khanacademy/kmath@2.2.30 ## @khanacademy/perseus-score@8.2.6 ### Patch Changes - Updated dependencies \[[`2d57e51583`](2d57e51), [`ea593fef51`](ea593fe), [`9cba5197a2`](9cba519)]: - @khanacademy/perseus-core@23.0.0 - @khanacademy/kmath@2.2.30 Author: khan-actions-bot Reviewers: benchristel Required Reviewers: Approved By: benchristel Checks: ⏭️ 2 checks have been skipped, ✅ 6 checks were successful Pull Request URL: #3187
Summary:
This pull request refactors the Expression widget from a class-based component to a modern functional component using React hooks. This conversion improves the long-term maintainability of the component by aligning it with current React best practices, making the code easier to read and reason about. The test suite for the widget has also been significantly expanded with more comprehensive and robust tests, covering accessibility, focus management, and programmatic content insertion to prevent future regressions.
Key technical improvements include a more robust focus management system that handles edge cases gracefully, especially on mobile. A new wrapper component was introduced to create a stable interface for the underlying math input component, resolving previous fragility. Additionally, the Storybook stories and test data have been updated for better clarity and descriptiveness, making it easier for future engineers to understand and work with the component.
Summary: Gemini
Code: Claude
Reviews: Codex, Claude, Gemini
Issue: LEMS-3804
Test plan: