Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactoring Numeric Input helper functions to remove underscore #2128

Open
wants to merge 8 commits into
base: feature/numeric-dx-refactor
Choose a base branch
from
Open

Refactoring Numeric Input helper functions to remove underscore #2128

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
0be21bc
[numeric-no-underscore] Refactoring Numeric Input helper functions to…
SonicScrewdriver Jan 17, 2025
31d2245
[numeric-no-underscore] Updating parser to handle stricter types
SonicScrewdriver Jan 17, 2025
9ea9033
[numeric-no-underscore] docs(changeset): Refactoring Numeric Input he…
SonicScrewdriver Jan 17, 2025
a509e4c
[numeric-no-underscore] Updating function name and comment to indicat…
SonicScrewdriver Jan 17, 2025
bf17209
[numeric-no-underscore] Remove old import
SonicScrewdriver Jan 20, 2025
c907b26
[numeric-no-underscore] Updating snapshots after modernizing tests an…
SonicScrewdriver Jan 20, 2025
1051dc3
[numeric-no-underscore] Simplifying logic for getUniqueAnswerForms, a…
SonicScrewdriver Jan 20, 2025
ad871e0
[numeric-no-underscore] PR Feedback
SonicScrewdriver Jan 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/sharp-peaches-love.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
"@khanacademy/math-input": minor
"@khanacademy/perseus": minor
"@khanacademy/perseus-core": minor
---

Refactoring Numeric Input helper functions to remove underscore, improve documentation, and add tests.
16 changes: 8 additions & 8 deletions packages/perseus-core/src/data-schema.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1164,16 +1164,16 @@ export type MathFormat =
| "pi";

export type PerseusNumericInputAnswerForm = {
simplify:
| "required"
| "correct"
| "enforced"
| "optional"
| null
| undefined;
simplify: PerseusNumericInputSimplify | null | undefined;
name: MathFormat;
};

export type PerseusNumericInputSimplify =
| "required"
| "correct"
| "enforced"
| "optional";

export type PerseusNumericInputWidgetOptions = {
// A list of all the possible correct and incorrect answers
answers: ReadonlyArray<PerseusNumericInputAnswer>;
Expand Down Expand Up @@ -1210,7 +1210,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 | null | undefined;
};

export type PerseusNumberLineWidgetOptions = {
Expand Down
2 changes: 1 addition & 1 deletion packages/perseus/src/components/input-with-examples.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. :)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

? ""
: `${this.props.examples[0]}
${this.props.examples.slice(1).join(", or\n")}`
Expand Down
18 changes: 16 additions & 2 deletions packages/perseus/src/util/parse-perseus-json/perseus-parsers/numeric-input-widget.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,13 @@ const parseMathFormat = enumeration(
"pi",
);

const parseSimplify = enumeration(
"required",
"correct",
"enforced",
"optional",
);

export const parseNumericInputWidget: Parser<NumericInputWidget> = parseWidget(
constant("numeric-input"),
object({
Expand All @@ -48,8 +55,15 @@ export const parseNumericInputWidget: Parser<NumericInputWidget> = parseWidget(
// the data, we should simplify `simplify`.
simplify: optional(
nullable(
union(string).or(
pipeParsers(boolean).then(convert(String)).parser,
union(parseSimplify).or(
pipeParsers(boolean).then(
convert((value) => {
if (typeof value === "boolean") {
return value ? "required" : "optional";
}
return value;
}),
).parser,
).parser,
),
),
Expand Down
4 changes: 2 additions & 2 deletions ...arse-perseus-json/regression-tests/__snapshots__/parse-perseus-json-snapshot.test.ts.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6490,7 +6490,7 @@ exports[`parseAndTypecheckPerseusItem correctly parses data/numeric-input-answer
"answerForms": undefined,
"maxError": 0,
"message": "",
"simplify": "true",
"simplify": "required",
"status": "correct",
"strict": false,
"value": 1.125,
Expand Down Expand Up @@ -7071,7 +7071,7 @@ exports[`parseAndTypecheckPerseusItem correctly parses data/numeric-input-with-s
"answerForms": undefined,
"maxError": 0,
"message": "",
"simplify": "false",
"simplify": "optional",
"status": "correct",
"strict": false,
"value": 2.6,
Expand Down
Loading
Loading