Matrix: renderer user input state #2581
Conversation
|
Size Change: -28 B (-0.01%) Total Size: 473 kB
ℹ️ View Unchanged
|
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (a4c8cc4) and published it to npm. You Example: pnpm add @khanacademy/perseus@PR2581If you are working in Khan Academy's frontend, you can run the below command. ./tools/bump_perseus_version.js -t PR2581If you are working in Khan Academy's webapp, you can run the below command. ./dev/tools/bump_perseus_version.js -s n -t PR2581 |
| @@ -37,7 +38,7 @@ class MatrixEditor extends React.Component<Props> { | |||
| static defaultProps: MatrixDefaultWidgetOptions = | |||
| matrixLogic.defaultWidgetOptions; | |||
|
|
|||
| change: (arg1: any, arg2: any, arg3: any) => any = (...args) => { | |||
| change: (arg1: any, arg2?: any, arg3?: any) => any = (...args) => { | |||
There was a problem hiding this comment.
Again, it'd be nice to remove changeable here too, but it's not required.
There was a problem hiding this comment.
I can't get rid of it everywhere right now because there are some uses that are beyond the scope of my current work. In this case the widget isn't wrapped in a Renderer so the editor is handling its own state. Maybe something that should change, but not related to my current work.
| onBlur: () => {}, | ||
| onFocus: () => {}, | ||
| trackInteraction: () => {}, | ||
| const matrixProps: Partial<PropsFor<typeof Matrix>> = { |
There was a problem hiding this comment.
I've already mentioned this separately in other PR's, but I wonder if we couldn't simplify how we're doing these types since we're making a partial and then casting it as the full props later.
There was a problem hiding this comment.
From the other PR:
I don't know what to do about the cast on 317. We're expecting props for an Expression widget but the editor doesn't provide all of those props. This happens in a couple of places in the editors because the widgets in the editors don't need all of the stuff they need in the learner experience. For the most part this worked because of poor type coverage or use of
ts-expect-errorwhich is much worse than type casting.I think this is still an improvement than what was here before and I think further work on this is beyond the scope of my current work.
| type RenderProps = MatrixPublicWidgetOptions & { | ||
| emptyMatrix: ReadonlyArray<ReadonlyArray<string>>; | ||
| }; | ||
| // type RenderProps = MatrixPublicWidgetOptions; |
There was a problem hiding this comment.
Should you clear this line out, given we have the proper declaration right after? If not, we might want to add a comment explaining why we're keeping this here.
| @@ -104,29 +83,32 @@ type ExternalProps = WidgetProps< | |||
| PerseusMatrixUserInput | |||
| > satisfies PropsFor<typeof Matrix>; | |||
|
|
|||
| type RenderProps = MatrixPublicWidgetOptions & { | |||
| emptyMatrix: ReadonlyArray<ReadonlyArray<string>>; | |||
There was a problem hiding this comment.
I see we're losing this emptyMatrix prop - was it unused?
| const {matrixBoardSize, prefix, suffix} = widgetOptions; | ||
| return { | ||
| matrixBoardSize, | ||
| prefix, | ||
| suffix, | ||
| emptyMatrix: blankInput, |
There was a problem hiding this comment.
Oh I see, here it is! Seems like a decent cleanup considering the tests are proving the inputs are properly empty. What a strange thing that doesn't seem to have been helping much at all.
Summary:
See the parent PR for more context. Part of LEMS-3208.
In #2566 I add additional APIs to let widgets consume
userInputfrom a parent and to update that parent state usinghandleUserInput, while also supporting the legacy way of storing user input (which was a combination of internal widget state and stashing state in a random blob in Renderer). This PR is part of a series of PRs that go widget-by-widget to use the new API.This is a generic summary I'm putting on each PR and not every point will be applicable to every widget, but the common themes are:
getSerializedStateandgetUserInputFromSerializedStateto keep supporting the expected results.userInputstate in Renderer. This means consuminguserInputin the component and callinghandleUserInputon change.transformdid something to initialize user input state, I moved the logic togetStartUserInput.staticTransformdid something to get correct state in static widgets, I moved the logic togetCorrectUserInput.Please see the parent PR for more information.
MatrixEditor does use Matrix, so this will affect the editing experience