reviewModeRubric prework: limit reviewModeRubric uses #1741

handeyeco · 2024-10-11T15:42:20Z

Summary:

Not sure we'll want to merge this as it might be too risky/dirty, but I thought I'd try to remove as many instances as I could of reviewModeRubric without really changing logic to make it clearer where we're actually using it. Seems to just be radio.

Issue: Prework for LEMS-2437

github-actions · 2024-10-11T15:45:45Z

Size Change: -54 B (-0.01%)

Total Size: 867 kB

Filename	Size	Change
`packages/perseus-editor/dist/es/index.js`	281 kB	+5 B (0%)
`packages/perseus/dist/es/index.js`	419 kB	-59 B (-0.01%)

ℹ️ View Unchanged

Filename	Size
`packages/kas/dist/es/index.js`	38.8 kB
`packages/keypad-context/dist/es/index.js`	760 B
`packages/kmath/dist/es/index.js`	4.27 kB
`packages/math-input/dist/es/index.js`	77.7 kB
`packages/math-input/dist/es/strings.js`	1.79 kB
`packages/perseus-core/dist/es/index.js`	1.48 kB
`packages/perseus-linter/dist/es/index.js`	22.2 kB
`packages/perseus/dist/es/strings.js`	3.4 kB
`packages/pure-markdown/dist/es/index.js`	3.66 kB
`packages/simple-markdown/dist/es/index.js`	12.4 kB

_{compressed-size-action}

github-actions · 2024-10-11T15:48:00Z

npm Snapshot: Published

Good news!! We've packaged up the latest commit from this PR (1501379) and published it to npm. You
can install it using the tag PR1741.

Example:

yarn add @khanacademy/perseus@PR1741

If you are working in Khan Academy's webapp, you can run:

./dev/tools/bump_perseus_version.sh -t PR1741

handeyeco · 2024-10-15T14:15:32Z

packages/perseus/src/types.ts

+    reviewModeRubric?: Rubric | null | undefined;
+    reviewMode: boolean;


As far as I could tell, the only widget that was really using the contents of reviewModeRubric was Radio. The other widgets were just using its existence to tell if they were in review mode or not. Then if you looked at how reviewModeRubric was set, it kind of seemed like needless misdirection:

// renderer.tsx checks if we're in `reviewMode` then sets `reviewModeRubric` const reviewModeRubric = this.props.reviewMode && widgetInfo ? widgetInfo.options : null; // passage.tsx for example then converted that object into a bool const editable = !this.props.reviewModeRubric;

So the crux of this PR is: why don't we just use reviewMode everywhere that's using coercing reviewModeRubric to a flag so that it's clearer what's actually using reviewModeRubric when we start work on LEMS-2437.

handeyeco · 2024-10-15T14:16:45Z

packages/perseus/src/types.ts

@@ -673,7 +673,8 @@ export type WidgetProps<
    onFocus: (blurPath: FocusPath) => void;
    onBlur: (blurPath: FocusPath) => void;
    findWidgets: (criterion: FilterCriterion) => ReadonlyArray<Widget>;
-    reviewModeRubric: Rubric;


I think this was mistyped.

const reviewModeRubric = this.props.reviewMode && widgetInfo ? widgetInfo.options : null;

handeyeco · 2024-10-15T14:20:12Z

packages/perseus-editor/src/widgets/input-number-editor.tsx

@@ -46,14 +48,12 @@ const answerTypes = {

 type Props = {
    value: number;
-    simplify: PropsFor<typeof InputNumber.widget>["simplify"];
-    size: PropsFor<typeof InputNumber.widget>["size"];
-    inexact: PropsFor<typeof InputNumber.widget>["reviewModeRubric"]["inexact"];


TS was exploding since reviewModeRubric could have been null/undefined. This might be an unpopular opinion, but I'm not a big fan of the PropsFor<typeof SomeReactComponent> pattern anyway. The editor exists to set widget options, so I feel like this is more correct anyway?

I actually agree. I think the widget options types should be the glue between the editors and their widgets. I think we've gone through iterations of understanding Perseus code and so this might have been me doing this even (probably because the widget option type wasn't exported (yet)).

khan-actions-bot · 2024-10-15T14:21:12Z

Gerald

Required Reviewers

@Khan/perseus for changes to .changeset/six-clocks-buy.md, packages/perseus/src/renderer.tsx, packages/perseus/src/types.ts, packages/perseus-editor/src/widgets/input-number-editor.tsx, packages/perseus/src/widgets/expression/expression.stories.tsx, packages/perseus/src/widgets/group/group.tsx, packages/perseus/src/widgets/input-number/input-number.tsx, packages/perseus/src/widgets/numeric-input/numeric-input.stories.tsx, packages/perseus/src/widgets/passage/passage.tsx, packages/perseus/src/widgets/radio/base-radio.tsx, packages/perseus/src/widgets/radio/radio-component.tsx, packages/perseus-editor/src/widgets/radio/editor.tsx, packages/perseus/src/widgets/passage/__tests__/passage.test.tsx, packages/perseus/src/widgets/radio/__stories__/base-radio.stories.tsx

Don't want to be involved in this pull request? Comment #removeme and we won't notify you of further changes.

jeremywiebe

Thanks. I'm in favour!

jeremywiebe · 2024-10-15T22:47:35Z

packages/perseus-editor/src/widgets/input-number-editor.tsx

@@ -46,14 +48,12 @@ const answerTypes = {

 type Props = {
    value: number;
-    simplify: PropsFor<typeof InputNumber.widget>["simplify"];
-    size: PropsFor<typeof InputNumber.widget>["size"];
-    inexact: PropsFor<typeof InputNumber.widget>["reviewModeRubric"]["inexact"];


I actually agree. I think the widget options types should be the glue between the editors and their widgets. I think we've gone through iterations of understanding Perseus code and so this might have been me doing this even (probably because the widget option type wasn't exported (yet)).

jeremywiebe

This is a great refactoring (and simplification)! Thank-you.

jeremywiebe · 2024-10-16T23:54:45Z

packages/perseus/src/types.ts

@@ -673,7 +673,8 @@ export type WidgetProps<
    onFocus: (blurPath: FocusPath) => void;
    onBlur: (blurPath: FocusPath) => void;
    findWidgets: (criterion: FilterCriterion) => ReadonlyArray<Widget>;
-    reviewModeRubric: Rubric;
+    reviewModeRubric?: Rubric | null | undefined;


Agreed with one tiny nit: If we have this typed with ? then | undefined is redundant.

handeyeco added 2 commits October 11, 2024 10:30

refine uses of reviewModeRubric

6762e06

move what I can to reviewMode

99c3f5c

handeyeco self-assigned this Oct 11, 2024

handeyeco changed the title ~~refine uses of reviewModeRubric~~ WIP/POC: reviewModeRubric prework Oct 11, 2024

changeset

286e515

handeyeco commented Oct 15, 2024

View reviewed changes

handeyeco marked this pull request as ready for review October 15, 2024 14:20

handeyeco requested a review from jeremywiebe October 15, 2024 14:21

khan-actions-bot requested a review from a team October 15, 2024 14:21

handeyeco changed the title ~~WIP/POC: reviewModeRubric prework~~ reviewModeRubric prework: limit reviewModeRubric uses Oct 15, 2024

handeyeco removed the request for review from a team October 15, 2024 14:21

khan-actions-bot requested a review from a team October 15, 2024 14:21

jeremywiebe approved these changes Oct 15, 2024

View reviewed changes

jeremywiebe approved these changes Oct 17, 2024

View reviewed changes

respond to Jeremys feedback

1501379

handeyeco merged commit 3e48b2c into main Oct 22, 2024
9 checks passed

handeyeco deleted the LEMS-2437/review-mode-rubric-prework branch October 22, 2024 21:27

khan-actions-bot mentioned this pull request Oct 22, 2024

Version Packages #1767

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

reviewModeRubric prework: limit reviewModeRubric uses #1741

reviewModeRubric prework: limit reviewModeRubric uses #1741

handeyeco commented Oct 11, 2024 •

edited

Loading

github-actions bot commented Oct 11, 2024 •

edited

Loading

github-actions bot commented Oct 11, 2024 •

edited

Loading

handeyeco Oct 15, 2024

handeyeco Oct 15, 2024

handeyeco Oct 15, 2024

jeremywiebe Oct 15, 2024

khan-actions-bot commented Oct 15, 2024

jeremywiebe left a comment

jeremywiebe Oct 15, 2024

jeremywiebe left a comment

jeremywiebe Oct 16, 2024

		reviewModeRubric?: Rubric \| null \| undefined;
		reviewMode: boolean;

reviewModeRubric prework: limit reviewModeRubric uses #1741

reviewModeRubric prework: limit reviewModeRubric uses #1741

Conversation

handeyeco commented Oct 11, 2024 • edited Loading

Summary:

github-actions bot commented Oct 11, 2024 • edited Loading

github-actions bot commented Oct 11, 2024 • edited Loading

npm Snapshot: Published

handeyeco Oct 15, 2024

Choose a reason for hiding this comment

handeyeco Oct 15, 2024

Choose a reason for hiding this comment

handeyeco Oct 15, 2024

Choose a reason for hiding this comment

jeremywiebe Oct 15, 2024

Choose a reason for hiding this comment

khan-actions-bot commented Oct 15, 2024

Gerald

jeremywiebe left a comment

Choose a reason for hiding this comment

jeremywiebe Oct 15, 2024

Choose a reason for hiding this comment

jeremywiebe left a comment

Choose a reason for hiding this comment

jeremywiebe Oct 16, 2024

Choose a reason for hiding this comment

handeyeco commented Oct 11, 2024 •

edited

Loading

github-actions bot commented Oct 11, 2024 •

edited

Loading

github-actions bot commented Oct 11, 2024 •

edited

Loading