[Radio] Merge top-level widget files into one, and refactor to remove legacy code#3273
[Radio] Merge top-level widget files into one, and refactor to remove legacy code#3273mark-fitzgerald wants to merge 13 commits intomainfrom
Conversation
…adio.ff.tsx into multiple-choice-widget.tsx. Start working through TypeScript errors caused by changes.
Update unit tests.
Adjust getPromptJSON to use the randomized choices.
…e top-level widget files into one, and refactor to remove legacy code
🗄️ Schema Change: No Changes ✅ |
🛠️ Item Splitting: No Changes ✅ |
|
Size Change: -272 B (-0.06%) Total Size: 485 kB
ℹ️ View Unchanged
|
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (4f249f8) and published it to npm. You Example: pnpm add @khanacademy/perseus@PR3273If you are working in Khan Academy's frontend, you can run the below command. ./dev/tools/bump_perseus_version.ts -t PR3273If you are working in Khan Academy's webapp, you can run the below command. ./dev/tools/bump_perseus_version.js -t PR3273 |
…oWidgetHandle (only used within the file).
| readOnly: false, | ||
| selected: false, | ||
| }, | ||
| {selected: false}, |
There was a problem hiding this comment.
I would be cautious changing the result from getSerializedState.
It is deprecated and we've told people it is unstable, but there are still consumers around until probably 2027.
A possible alternative is to keep the shape of the return value of getSerializedState and fill it with placeholder values in Radio's implementation of getSerializedState.
There was a problem hiding this comment.
@handeyeco Do we have a ticket to fully removing the deprecated function? maybe we can add a todo here attributed to that work in the future, for easier search.
There was a problem hiding this comment.
| correct: boolean; | ||
| isNoneOfTheAbove: boolean; | ||
| previouslyAnswered: boolean; | ||
| // TODO(LEMS-3783): remove uses of `revealNoneOfTheAbove` | ||
| revealNoneOfTheAbove: boolean; |
There was a problem hiding this comment.
Does this fulfill https://khanacademy.atlassian.net/browse/LEMS-3783 or is there still more to do?
There was a problem hiding this comment.
I found one other instance of that TODO in perseus/src/types.ts. I'll remove it from there, as well.
| return this.radioRef.current.getPromptJSON(); | ||
| } | ||
|
|
||
| _mergePropsAndState(): Props & { |
There was a problem hiding this comment.
We don't need _mergePropsAndState anymore because we don't use the old state anymore right? It's all props now? I think we talked about doing this, just confirming.
There was a problem hiding this comment.
That is correct.
| choiceStates: [ | ||
| { | ||
| selected: true, // <= note we stash user input | ||
| highlighted: false, |
There was a problem hiding this comment.
This is probably where I should have put that other comment: serialize-[widget].test.ts tests should be changed with caution.
| // Export as Radio for backwards compatibility until we can | ||
| // perform Content Backfills to officially rename the Radio Widget | ||
| const Radio = MultipleChoiceWidget; | ||
| class Radio extends React.Component<Props> implements Widget { |
There was a problem hiding this comment.
As an outsider to the Radio project, I find it confusing that we seem to use "multiple choice widget" and "radio" interchangeably. I would advocate either:
- (Preferred) Only use "radio" since it's well known by the rest of the org
- Only use "multiple choice widget" so that we are consistent
This file is a good example of why this is confusing: the main export is Radio and the norm in React is to find that in radio.tsx (or index.tsx but I think it's even more confusing to have 5 index.tsx files open at once). However it's found in radio/multiple-choice-widget.tsx...it's kind of painful writing that.
Where do I find the tests for Radio? multiple-choice-widget.test.tsx obviously. Where do I find test data for multiple-choice.test.ts? radio.testdata.ts!
…es in getSerializedState output so that consumers aren't disrupted by it. These will be fully removed once LEMS-3185 is completed.
…roperty to aid in PR review.
| handleUserInput({ | ||
| selectedChoiceIds: checkedChoiceIds, | ||
| }); | ||
|
|
||
| trackInteraction(); | ||
| announceChoiceChange(newChoiceStates); | ||
| announceChoiceChange(choiceStates); |
There was a problem hiding this comment.
Will calling announceChoiceChange with choiceStates pulled from props.userInput result in a stale/incorrect SR announcement?
I don't think handleUserInput will update the userInput before the announcement runs
There was a problem hiding this comment.
Correct. More specifically, the logic for generating the choices in the props for this file has now been moved to this file. So, this announcement function should be pulling from the new object in this file.
| editMode?: boolean; | ||
| labelWrap?: boolean; | ||
| randomize?: boolean; | ||
| onChoiceChange: (choiceId: string, newCheckedState: boolean) => void; |
There was a problem hiding this comment.
Do we need onChoiceChange on RadioProps? It looks like the widget computes onChoiceChange internally (based on apiOptions.readOnly / review mode) and never reads props.onChoiceChange. Can we remove it from the props type to avoid confusion?
Also, somewhat related, there are a few props that don't seem to be used at all, we might be able to remove them, like editMode and labelWrap
There was a problem hiding this comment.
Good catch on the onChoiceChange. I added it for a reason, originally. Looks like that reason got resolved after further refactoring.
There was a problem hiding this comment.
Removed those other two props, a well.
There was a problem hiding this comment.
I think your updates might still be local 😅
ivyolamit
left a comment
There was a problem hiding this comment.
Mark changes looks logical to me, going to do a smoke test next.
| @@ -100,13 +100,6 @@ export type EditorMode = "edit" | "preview" | "json"; | |||
|
|
|||
| export type ChoiceState = { | |||
| selected: boolean; | |||
There was a problem hiding this comment.
I remembered you brought this up before if we still need ChoiceState or not, do you remember what was the end result of that discussion?
There was a problem hiding this comment.
I imagine this could be refactored now that it is only a singular value (selected). However, it is used in a number of places, so pulling it out now would be beyond the scope of this ticket.
…-choice" instead of "radio", for consistency. Remove unused props: editMode, labelWrap, onChoiceChange.
anakaren-rojas
left a comment
There was a problem hiding this comment.
What an undertaking!
ivyolamit
left a comment
There was a problem hiding this comment.
Sorry for the delay, changes look good to me 🎉 ![]()
Summary:
This PR merges radio.ts, radio.ff.tsx, and multiple-choice-widget.tsx into a single file. It also refactors the code to remove as much of the legacy code as is possible at this time. This merging and refactoring removes the last of the Feature Flag code used during development and release.
Issue: LEMS-3849
Test plan: