[Radio] Remove old widget files #3199
Conversation
Update radio.ff.tsx to only use the new version of the widget.
…ntext to the top-level widget class (radio.ff.tsx) Update unit tests for new widget details.
Remove radio.test.ts as all of its tests are now in multiple-choice.test.ts. Update unit tests to reference multiple choice options correctly (role = "button") and to check for its name correctly (name: /(Choice A)/).
mark-fitzgerald
requested review from
Myranae,
anakaren-rojas,
handeyeco and
ivyolamit
🗄️ Schema Change: No Changes ✅ |
🛠️ Item Splitting: No Changes ✅ |
|
Size Change: -4.5 kB (-0.92%) Total Size: 483 kB
ℹ️ View Unchanged
|
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (500751e) and published it to npm. You Example: pnpm add @khanacademy/perseus@PR3199If you are working in Khan Academy's frontend, you can run the below command. ./dev/tools/bump_perseus_version.ts -t PR3199If you are working in Khan Academy's webapp, you can run the below command. ./dev/tools/bump_perseus_version.js -t PR3199 |
| }); | ||
|
|
||
| // LEMS-3445: Ensures that deselecting one choice doesn't select other choices | ||
| it("should not select other choices when deselecting current choice", async () => { |
There was a problem hiding this comment.
This used to be in the multiple-select section, but since it is using a single-select question, I moved it here.
| }); | ||
|
|
||
| // LEMS-3445: Ensures that deselecting one choice doesn't select other choices | ||
| it("should not select other choices when deselecting current choice - single select", async () => { |
There was a problem hiding this comment.
Moved to the single-select section above.
…t to actually look for the intended content (original change was not done correctly). Remove commented out line in index.ts.
ivyolamit
left a comment
ivyolamit
left a comment
There was a problem hiding this comment.
Overall PR looks good to me on my first pass, will take a look again first thing tomorrow morning and do a smoke test.
I did noticed in the storybook we still have these two
|
|
||
| constructor(props: Props) { | ||
| super(props); | ||
| this.ffIsOn = props.apiOptions.flags?.["new-radio-widget"] ?? false; |
| if (!radioInput) { | ||
| throw new Error( | ||
| `Could not find radio input for choice content: ${choice.content}`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Should we be using getBy* queries and assertions instead of conditionals / manual throws? This looks like an anti-pattern that existed in the file already, but wanted to flag it so we can avoid continuing it.
There was a problem hiding this comment.
The intent of this test is to activate the button associated with the given choice. We only know the text of the choice, not the letter. Therefore, I used queryByText to find the choice content, and then navigated to the button associated with it. I wasn't confident that clicking the found text would trigger the onClick event handler that we have on the <li> tag. After testing that theory, it appears to work properly. I will update the test to not use the DOM queries.
… to click the found choice text instead of searching for the associated button. Update the Storybook files to have a single "Radio" section.
ivyolamit
left a comment
ivyolamit
left a comment
There was a problem hiding this comment.
@mark-fitzgerald going to approve this PR changes looks logical to me and smoke testing 👌🏼
Great work on this! 👏🏼
…sert non-null states instead of using if statements. Format as AAA.
| // Assert | ||
| expect(widgetJSON.type).toEqual("radio"); | ||
|
|
There was a problem hiding this comment.
these tests are much better! 🥳
Summary:
Now that the feature flag is on for all apps, we can remove all code involed in the old version of the widget. This PR removes those old widget files, leaving the
*.new.tsxfiles. It also updates numerous unit tests to properly reference the multiple choice options.Issue: LEMS-3847
Test plan:
Unit tests pass
Visual regression tests pass