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

[its-a-numeric-story] Improving Numeric Input Storybook Stories #2138

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

Conversation

SonicScrewdriver
Copy link
Contributor

@SonicScrewdriver SonicScrewdriver commented Jan 22, 2025

Summary:

This PR is part of the Numeric Input Project.

The purpose of this PR is to improve our storybook setup for our Numeric Input Widget. This includes the following work:

  • Modernizing story structure
  • Hooking up argTypes with descriptions
  • Updating RendererWithDebugUI to also set customKeypad to the same value as isMobile
    - This allows us to ensure that our Widgets that use MathInput are properly updating when toggled into Mobile view
  • Updating SideBySide to automatically collapse the Perseus JSON view.

Current (Live) Storybook Example | PR Storybook Example

Issue: LEMS-2449

Test plan:

  • Ensure all tests pass + manual testing

Modernizing story, ensuring controls work, updating RendererWithDebugUI to also set customKeypad alongside isMobile, and updating SideBySide to hide messy JSON.

Issue: LEMS-2449
Test Plan: Ensure all tests pass + manual testing
@SonicScrewdriver SonicScrewdriver 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 (7ca1c5b) and published it to npm. You
can install it using the tag PR2138.

Example:

yarn add @khanacademy/perseus@PR2138

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

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

Copy link
Contributor

github-actions bot commented Jan 22, 2025

Size Change: 0 B

Total Size: 1.47 MB

ℹ️ 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.7 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/index.js 396 kB
packages/perseus/dist/es/strings.js 5.1 kB
packages/pure-markdown/dist/es/index.js 3.66 kB
packages/simple-markdown/dist/es/index.js 12.5 kB

compressed-size-action

jsonObject: any;
};

const SideBySide = ({
Copy link
Contributor Author

@SonicScrewdriver SonicScrewdriver Jan 23, 2025

Choose a reason for hiding this comment

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

After some discussion, it was determined that we really don't like this side-by-side view. I've renamed this to "SplitView" to be a little more generic (in case we ever want to change it back), and renamed the variables accordingly.

This moves the PerseusJSON below the Widget, which gives both the Widget and the JSON more room to breathe.

...apiOptions,
isMobile,
customKeypad: isMobile, // Use the mobile keypad for mobile
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Many of our stories would not respond to the "Mobile" toggle. This addition now ensures that all Widgets that use MathInput will properly render their mobile views.

<ReactJson
style={{marginTop: "10px"}}
quotesOnKeys={false}
enableClipboard={false}
collapsed={5}
collapsed={true}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This auto-collapses the PerseusJSON view so that it doesn't fill the page, and push the Storybook Controls "below the fold".

@@ -134,6 +175,7 @@ export const multipleAnswersWithDecimals: PerseusRenderer = {
value: 12.2,
simplify: "required",
message: "",
answerForms: ["decimal"],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Realized that these should be specifying their answerForms, just for consistency.

</label>
</div>
);
export const Default = (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

After setting up all the Storybook controls, I wasn't really sure if we needed any more stories as we can control most of these options on the fly now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah! Perhaps a story for each answerForm, as it IS a little frustrating to edit the json views for that. If possible, it'd be good to get an example of the pi answerForm with a note about how to trigger the "It seems like you've approximated pi" message.

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.

1 participant