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.

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

Create helper to build public widget options for expression #2139

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

Myranae
Copy link
Contributor

@Myranae Myranae commented Jan 22, 2025

Summary:

Adds a function that takes expression's full widget options and filters out answer data. It also adds this function to the widget's widget export and adds a test confirming the function does what is expected.

Issue: LEMS-2758

Test plan:

  • Confirm all checks pass
  • Confirm expression still works as expected

@Myranae Myranae self-assigned this Jan 22, 2025
Copy link
Contributor

github-actions bot commented Jan 22, 2025

npm Snapshot: Published

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

Example:

yarn add @khanacademy/perseus@PR2139

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

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

Copy link
Contributor

github-actions bot commented Jan 22, 2025

Size Change: +170 B (+0.01%)

Total Size: 1.47 MB

Filename Size Change
packages/perseus/dist/es/index.js 396 kB +170 B (+0.04%)
ℹ️ 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 86.8 kB
packages/math-input/dist/es/index.js 77.6 kB
packages/math-input/dist/es/strings.js 1.79 kB
packages/perseus-core/dist/es/index.js 27.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 113 kB
packages/perseus/dist/es/strings.js 5.37 kB
packages/pure-markdown/dist/es/index.js 3.66 kB
packages/simple-markdown/dist/es/index.js 12.5 kB

compressed-size-action

Comment on lines 6 to 17
/**
* For details on the individual options, see the
* PerseusOrdererWidgetOptions type
*/
type ExpressionPublicWidgetOptions = {
buttonSets: LegacyButtonSets;
functions: ReadonlyArray<string>;
times: boolean;
visibleLabel?: string;
ariaLabel?: string;
buttonsVisible?: "always" | "never" | "focused";
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Left out answerFrom from the below widget options:

    // The expression forms the answer may come in
    answerForms: ReadonlyArray<PerseusExpressionAnswerForm>;
    buttonSets: LegacyButtonSets;
    // Variables that can be used as functions.  Default: ["f", "g", "h"]
    functions: ReadonlyArray<string>;
    // Use x for rendering multiplication instead of a center dot.
    times: boolean;
    // visible label associated with the MathQuill field
    visibleLabel?: string;
    // aria label for screen readers attached to MathQuill field
    ariaLabel?: string;
    // Controls when buttons for special characters are visible when using a
    // desktop browser.  Defaults to "focused".
    // NOTE: This isn't listed in perseus-format.js or perseus_data.go, but
    // appears in item data in the datastore.
    buttonsVisible?: "always" | "never" | "focused";

And here is PerseusExpressionAnswerForm. I don't think I need to pick out value from this, but if someone could confirm for me, that would be great.

value: string     
form: boolean     
simplify: boolean     
considered: typeof PerseusExpressionAnswerFormConsidered[number]     
key?: string 

@Myranae Myranae marked this pull request as ready for review January 22, 2025 22:15
Comment on lines +9 to +10
const options: PerseusExpressionWidgetOptions =
expressionItem2.question.widgets["expression 1"].options;
Copy link
Member

Choose a reason for hiding this comment

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

It looks like visibleLabel and ariaLabel are not included in the test data? I'd prefer to define the input inline rather than relying on shared data from expression.testdata.

Sharing data between tests makes them harder to understand (you have to look at multiple files to understand the test setup) and prone to false passes if the test data changes later.

Suggested change
const options: PerseusExpressionWidgetOptions =
expressionItem2.question.widgets["expression 1"].options;
const options: PerseusExpressionWidgetOptions = {
buttonSets: ["basic"],
functions: ["f", "g", "h"],
times: false,
visibleLabel: "the visible label",
ariaLabel: "the aria label",
buttonsVisible: "always",
};

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants