Refactoring Numeric Input helper functions to remove underscore #2128

SonicScrewdriver · 2025-01-17T23:16:40Z

Summary:

This PR is part of the Numeric Input Project, and will be landed onto the feature branch for a full QA pass.

The intended goal was to remove all cases of underscore in the Numeric Input component, and to improve the commenting / documentation of the code.

Some things to note:

I've changed the logic of generateExamples to match shouldShowExamples, as we were generating a list of all possible examples and simply not displaying it. This seemed unnecessary and we can exit both functions early.
I've added more specific types for PerseusNumericInputWidgetOptions.simplify

Issue: LEMS-2446

Test plan:

New tests
Manual testing in storybook

… remove underscore, improve documentation, and add tests. Issue: LEMS-2446

…lper functions to remove underscore, improve documentation, and add tests.

SonicScrewdriver · 2025-01-17T23:18:22Z

packages/perseus-core/src/data-schema.ts

@@ -1210,7 +1212,7 @@ export type PerseusNumericInputAnswer = {
    // NOTE: perseus_data.go says this is non-nullable even though we handle null values.
    maxError: number | null | undefined;
    // Unsimplified answers are Ungraded, Accepted, or Wrong. Options: "required", "correct", or "enforced"
-    simplify: string | null | undefined;
+    simplify: PerseusNumericInputSimplify;


This could be a spicy move, but I think it's good to be more specific with our types here. While it resulted in some minor downstream updates, I think greater specificity will help us.

Merging the | null | undefined into the type feels like it hides something important. Without referring to the type, this field looks required, but you find out that it's optional only when you inspect the new PerseusNumericInputSimplify.

I'm not sure if we have a precedent, but I think I'd prefer we kept the new type as the union of string values and then add | null | undefined here so that the "optionality" stays in situ.

SonicScrewdriver · 2025-01-17T23:19:02Z

packages/perseus/src/widgets/numeric-input/numeric-input.class.tsx

-    }>;
-    labelText: string;
-    size: "normal" | "small";
+    answerForms: ReadonlyArray<PerseusNumericInputAnswerForm>;


We already have this type, no need to force ourselves to maintain two locations.

SonicScrewdriver · 2025-01-17T23:19:53Z

packages/perseus/src/widgets/numeric-input/numeric-input.test.ts

@@ -376,35 +374,3 @@ describe("Numeric input widget", () => {
        );
    });
 });
-
-describe("unionAnswerForms utility function", () => {


I created better tests over in utils.test.ts for this logic.

github-actions · 2025-01-17T23:20:23Z

npm Snapshot: Published

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

Example:

yarn add @khanacademy/perseus@PR2128

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

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

github-actions · 2025-01-17T23:20:32Z

Size Change: -70 B (0%)

Total Size: 1.47 MB

Filename	Size	Change
`packages/perseus/dist/es/index.js`	413 kB	-70 B (-0.02%)

ℹ️ View Unchanged

Filename	Size
`packages/kas/dist/es/index.js`	39 kB
`packages/keypad-context/dist/es/index.js`	760 B
`packages/kmath/dist/es/index.js`	83.1 kB
`packages/math-input/dist/es/index.js`	78.1 kB
`packages/math-input/dist/es/strings.js`	1.79 kB
`packages/perseus-core/dist/es/index.js`	23.1 kB
`packages/perseus-editor/dist/es/index.js`	689 kB
`packages/perseus-linter/dist/es/index.js`	22.2 kB
`packages/perseus-score/dist/es/index.js`	103 kB
`packages/perseus/dist/es/strings.js`	5.04 kB
`packages/pure-markdown/dist/es/index.js`	3.66 kB
`packages/simple-markdown/dist/es/index.js`	12.5 kB

_{compressed-size-action}

SonicScrewdriver · 2025-01-17T23:20:32Z

packages/perseus/src/widgets/numeric-input/utils.test.ts

@@ -15,7 +16,7 @@ describe("generateExamples", () => {
                simplify: "required",
            },
        ];
-        const strings: Partial<PerseusStrings> = {
+        const fakePerseusStrings: Partial<PerseusStrings> = {


Mostly a self nit, but it seemed good to be really specific that I'm faking the strings here.

question: could we not use the mock strings from

perseus/packages/perseus/src/strings.ts

Line 551 in 9cabe68

export const mockStrings: PerseusStrings = {

?

…e that it's no longer generic as it was not being used elsewhere.

mark-fitzgerald · 2025-01-18T02:45:07Z

packages/perseus/src/widgets/numeric-input/utils.ts

+ * answer forms. This is useful for ensuring that we don't show duplicate examples
+ * to the user.
+ */
+const getUniqueAnswerForms = function (


I wonder if it would be simpler to implement something like how number line tick marks are handled? Perhaps something like:

const allFormNames = list.map((form) => form.name); const uniqueFormNames = [...new Set(allFormNames)]; return uniqueFormNames.map((name) => list.find((form) => form.name === name));

Not sure if simplify is needed in this evaluation.

I modded my approach slightly but you're right, this could be simplified even more! :)

This did make me think deeper about which examples we're filtering: the way we've historically generated these examples seems antithetical to how we intend Content Creators to create answers.

Say I want to make a course focused on fractions, and I don't want kids simply entering decimals. If I make a wrong answer that's strictly set to match a decimal, and a right answer set to strictly match a fraction, the provided tooltip will read: "Your answer should be - a simplified proper fraction - an exact decimal like 0.75". This would directly mislead the user as their answer specifically shouldn't be a decimal.🤔

This is how the logic works currently so it's not a regression. It does make me wonder if we should only be providing the correct answers to the tooltip however. It seems like an easy improvement that we could make. Perhaps a ticket for emergent work?

Edit: Actually, it might be nice to have a section for "Your answer should not be".

Agreed. Perhaps we can get confirmation from Content folks that only correct answers should be included in the tooltip? Though, I wonder if the scoring logic would fail when a "wrong" answer matches a "correct" answer (i.e. the decimal equivalent of a fraction).

SonicScrewdriver · 2025-01-20T20:20:23Z

packages/perseus/src/widgets/numeric-input/utils.ts

+    // If the Content Creator has not specified any answer forms,
+    // we do not need to show any examples.
+    if (answerForms.length === 0) {
+        return [];


I wanted to call out that I've technically changed this functionality.

Previously we would return a string with all of the example strings that we just never displayed. This seemed like unnecessary logic so I've updated it to simply exit early in such a case.

…d no longer generating example strings if we aren't going to show them.

…s we don't need the full scope of what isEqual was providing previously. <see below>

jeremywiebe

Sorry... drive by review comments. 🫶

jeremywiebe · 2025-01-20T23:14:37Z

packages/perseus-core/src/data-schema.ts

@@ -1210,7 +1212,7 @@ export type PerseusNumericInputAnswer = {
    // NOTE: perseus_data.go says this is non-nullable even though we handle null values.
    maxError: number | null | undefined;
    // Unsimplified answers are Ungraded, Accepted, or Wrong. Options: "required", "correct", or "enforced"
-    simplify: string | null | undefined;
+    simplify: PerseusNumericInputSimplify;


Merging the | null | undefined into the type feels like it hides something important. Without referring to the type, this field looks required, but you find out that it's optional only when you inspect the new PerseusNumericInputSimplify.

I'm not sure if we have a precedent, but I think I'd prefer we kept the new type as the union of string values and then add | null | undefined here so that the "optionality" stays in situ.

jeremywiebe · 2025-01-20T23:16:59Z

packages/perseus/src/components/input-with-examples.tsx

@@ -93,7 +93,7 @@ class InputWithExamples extends React.Component<Props, State> {
        const ariaId = `aria-for-${id}`;
        // Generate text from a known set of format options that will read well in a screen reader
        const examplesAria =
-            this.props.examples.length === 7
+            this.props.examples.length === 0


The original 7 feels very magic. Why are we now changing it to 0?

Previously we were looping through to return all of the example strings when the teacher had not selected any answerForms. Then we would never show these strings as shouldShowExamples would hide them in this case.

There are 6 example strings, and then we prepend a "Your answer should be" as the first string (for a total of 7). In the case where teachers have selected all 6 answerForms, we still don't show these answerForms as they are effectively useless.

const forms = this.props.answerForms?.length !== 0 ? this.props.answerForms : Object.keys(formExamples).map((name) => { return { name: name, simplify: "required", } as PerseusNumericInputAnswerForm; });

That seemed a little silly to me, so I've updated the function to exit early in such a case, returning an empty array. That meant that I could update this section to make more sense. :)

Ohhhh... Nice. Thanks for cleaning that up!

jeremywiebe · 2025-01-20T23:28:49Z

packages/perseus/src/widgets/numeric-input/numeric-input.class.tsx

+    const rendererProps: RenderProps & {answers?: any} = {
+        ...widgetOptions,
        answerForms: unionAnswerForms(
-            // Pull out the name of each form and whether that form has
-            // required simplification.
            widgetOptions.answers.map((answer) => {
-                // @ts-expect-error - TS2345 - Argument of type 'readonly MathFormat[] | undefined' is not assignable to parameter of type 'Collection<any>'.
-                return _.map(answer.answerForms, (form) => {
+                return (answer.answerForms || []).map((form) => {
                    return {
                        simplify: answer.simplify,
                        name: form,
                    };
                });
            }),
        ),
-    });
+    };

+    delete rendererProps.answers;


Instead of needing to jump through this typing hoop, what do you think of restructuring the answers away?

const {answers: _, ...rendererProps} = { ...widgetOptions, answerForms: unionAnswerForms( widgetOptions.answers.map((answer) => { return (answer.answerForms || []).map((form) => { return { simplify: answer.simplify, name: form, }; }); }), ), };

Much better, thank you!

jeremywiebe · 2025-01-20T23:34:05Z

packages/perseus/src/widgets/numeric-input/utils.test.ts

@@ -15,7 +16,7 @@ describe("generateExamples", () => {
                simplify: "required",
            },
        ];
-        const strings: Partial<PerseusStrings> = {
+        const fakePerseusStrings: Partial<PerseusStrings> = {


question: could we not use the mock strings from

perseus/packages/perseus/src/strings.ts

Line 551 in 9cabe68

export const mockStrings: PerseusStrings = {

?

SonicScrewdriver added 3 commits January 17, 2025 15:08

[numeric-no-underscore] Refactoring Numeric Input helper functions to…

0be21bc

… remove underscore, improve documentation, and add tests. Issue: LEMS-2446

[numeric-no-underscore] Updating parser to handle stricter types

31d2245

[numeric-no-underscore] docs(changeset): Refactoring Numeric Input he…

9ea9033

…lper functions to remove underscore, improve documentation, and add tests.

SonicScrewdriver self-assigned this Jan 17, 2025

SonicScrewdriver changed the title ~~[numeric-no-underscore] Refactoring Numeric Input helper functions to remove underscore, improve documentation, and add tests.~~ Refactoring Numeric Input helper functions to remove underscore Jan 17, 2025

SonicScrewdriver commented Jan 17, 2025

View reviewed changes

[numeric-no-underscore] Updating function name and comment to indicat…

a509e4c

…e that it's no longer generic as it was not being used elsewhere.

mark-fitzgerald reviewed Jan 18, 2025

View reviewed changes

[numeric-no-underscore] Remove old import

bf17209

SonicScrewdriver commented Jan 20, 2025

View reviewed changes

SonicScrewdriver added 2 commits January 20, 2025 13:09

[numeric-no-underscore] Updating snapshots after modernizing tests an…

c907b26

…d no longer generating example strings if we aren't going to show them.

[numeric-no-underscore] Simplifying logic for getUniqueAnswerForms, a…

1051dc3

…s we don't need the full scope of what isEqual was providing previously. <see below>

SonicScrewdriver marked this pull request as ready for review January 20, 2025 21:54

jeremywiebe reviewed Jan 20, 2025

View reviewed changes

[numeric-no-underscore] PR Feedback

ad871e0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring Numeric Input helper functions to remove underscore #2128

Refactoring Numeric Input helper functions to remove underscore #2128

SonicScrewdriver commented Jan 17, 2025 •

edited

Loading

SonicScrewdriver Jan 17, 2025

jeremywiebe Jan 20, 2025

SonicScrewdriver Jan 17, 2025

SonicScrewdriver Jan 17, 2025

github-actions bot commented Jan 17, 2025 •

edited

Loading

github-actions bot commented Jan 17, 2025 •

edited

Loading

SonicScrewdriver Jan 17, 2025

jeremywiebe Jan 20, 2025

mark-fitzgerald Jan 18, 2025 •

edited

Loading

SonicScrewdriver Jan 20, 2025

SonicScrewdriver Jan 20, 2025 •

edited

Loading

mark-fitzgerald Jan 21, 2025

SonicScrewdriver Jan 20, 2025 •

edited

Loading

jeremywiebe left a comment

jeremywiebe Jan 20, 2025

jeremywiebe Jan 20, 2025

SonicScrewdriver Jan 21, 2025

jeremywiebe Jan 21, 2025

jeremywiebe Jan 20, 2025

SonicScrewdriver Jan 21, 2025

jeremywiebe Jan 20, 2025

Refactoring Numeric Input helper functions to remove underscore #2128

Are you sure you want to change the base?

Refactoring Numeric Input helper functions to remove underscore #2128

Conversation

SonicScrewdriver commented Jan 17, 2025 • edited Loading

Summary:

Test plan:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 17, 2025 • edited Loading

npm Snapshot: Published

github-actions bot commented Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mark-fitzgerald Jan 18, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SonicScrewdriver Jan 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SonicScrewdriver Jan 20, 2025 • edited Loading

Choose a reason for hiding this comment

jeremywiebe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SonicScrewdriver commented Jan 17, 2025 •

edited

Loading

github-actions bot commented Jan 17, 2025 •

edited

Loading

github-actions bot commented Jan 17, 2025 •

edited

Loading

mark-fitzgerald Jan 18, 2025 •

edited

Loading

SonicScrewdriver Jan 20, 2025 •

edited

Loading

SonicScrewdriver Jan 20, 2025 •

edited

Loading