Remove score prop from ServerItemRenderer
#3101
Conversation
🗄️ Schema Change: No Changes ✅ |
🛠️ Item Splitting: No Changes ✅ |
|
Size Change: -245 B (-0.05%) Total Size: 498 kB
ℹ️ View Unchanged
|
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (bed332a) and published it to npm. You Example: pnpm add @khanacademy/perseus@PR3101If you are working in Khan Academy's frontend, you can run the below command. ./dev/tools/bump_perseus_version.ts -t PR3101If you are working in Khan Academy's webapp, you can run the below command. ./dev/tools/bump_perseus_version.js -t PR3101 |
| problemNum?: number; | ||
| questionCompleted?: boolean; | ||
| reviewMode?: boolean | null | undefined; | ||
| highlightEmptyWidgets?: boolean; |
There was a problem hiding this comment.
I'm actually not sure we ever highlight empty widgets at all. We only enable the "Check" button when we're "answerable" but we're only answerable when there are no empty widgets...
There was a problem hiding this comment.
I'm pretty sure that not all frontends want to behave this way (requiring everything filled in before "Check Answer" is enabled).
However, I don't think that should block this PR. Rather, we probably want to have a prop that we set to enable empty widget highlighting. Then the frontend could allow the "Check Answer" button and if the learner clicks it, the frontend could validate the user input and if it sees that client validation fails, it could turn on the empty widget highlighting.
| if (this.props.score && this.props.score !== prevProps.score) { | ||
| const emptyQuestionAreaWidgets = | ||
| this.questionRenderer.emptyWidgets(); | ||
|
|
||
| this.setState({ | ||
| questionCompleted: this.props.score.correct, | ||
| questionHighlightedWidgets: emptyQuestionAreaWidgets, | ||
| }); | ||
| } |
There was a problem hiding this comment.
I wanted to get rid of this because I don't think we really need score. I think this was really just a "did we attempt a problem?" flag. Then the things we did with that didn't really seem used?
handeyeco
changed the title
score prop from ServerItemRenderer
handeyeco
requested review from
SonicScrewdriver,
anakaren-rojas,
ivyolamit and
mark-fitzgerald
as code owners
jeremywiebe
left a comment
jeremywiebe
left a comment
There was a problem hiding this comment.
I'm very in favour of this!
Question: how do widgets show rationale's after these changes? Is that still supported?
| problemNum?: number; | ||
| questionCompleted?: boolean; | ||
| reviewMode?: boolean | null | undefined; | ||
| highlightEmptyWidgets?: boolean; |
There was a problem hiding this comment.
I'm pretty sure that not all frontends want to behave this way (requiring everything filled in before "Check Answer" is enabled).
However, I don't think that should block this PR. Rather, we probably want to have a prop that we set to enable empty widget highlighting. Then the frontend could allow the "Check Answer" button and if the learner clicks it, the frontend could validate the user input and if it sees that client validation fails, it could turn on the empty widget highlighting.
| widgetsEmpty, | ||
| ); | ||
| this.props.onInteractWithWidget(widgetId); | ||
| this.props.apiOptions?.interactionCallback?.(updatedUserInput); |
There was a problem hiding this comment.
🤔 I wonder... with the UserInputManager, would that fully replace the need for interaction callbacks today? (Not asking you to do that in this PR, but I wonder...)
| // finished rendering. | ||
| // TODO(jeff, CP-3128): Use Wonder Blocks Timing API | ||
| // eslint-disable-next-line no-restricted-syntax | ||
| setTimeout(() => { |
There was a problem hiding this comment.
These setTimeout()s throughout Perseus made/make testing annoying. Thanks for removing this unused function!
Summary:
I was looking at the
scoreprop for ServerItemRenderer. It seemed a little sus to me. It was setting two things:questionCompleted: this was only consumed in Radio, but I couldn't really understand what it's for. Didn't seem like we really needed it?questionHighlightedWidgets: this seemed to be highlighting empty widgets, but that didn't make a lot of sense to me. We only allow learners to click "Check" when the user input is deemed "answerable" but answerable is based on whether there's an empty widget or not. So it's impossible to highlight empty widgets after an attempt because an attempt is impossible with empty widgets?It would benefit SSS if it didn't need the score, so I pulled some threads.
TODOs are for LEMS-3783