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

View Locked Function in the Interactive Graph #1383

Merged

Conversation

mark-fitzgerald
Copy link
Contributor

@mark-fitzgerald mark-fitzgerald commented Jun 28, 2024

Summary:

Existing locked figures include lines that are straight. The need exists to also have lines that follow a function/equation. Plotting arbitrary functions as a locked figure will provide content creators with a powerful drawing tool for interactive graphs.

Issue: LEMS-1944

Test plan:

  1. Launch Storybook (yarn start)
  2. Navigate to Perseus => Widgets => Interactive Graph => Locked Functions
  3. Note that there are multiple stories available to demonstrate various functions and options

Examples:

Quadratic
Quadratic

Sine
Sine

Logarithmic
Logarithmic

Mark Fitzgerald and others added 5 commits June 24, 2024 15:39
Add test data for locked function.
Add Storybook stories for locked function.
Add tests.
Add unit test for plot axis references.
@mark-fitzgerald mark-fitzgerald self-assigned this Jun 28, 2024
@mark-fitzgerald mark-fitzgerald requested a review from a team June 28, 2024 16:04
Copy link
Contributor

github-actions bot commented Jun 28, 2024

Size Change: +268 B (+0.03%)

Total Size: 849 kB

Filename Size Change
packages/perseus-editor/dist/es/index.js 272 kB +114 B (+0.04%)
packages/perseus/dist/es/index.js 410 kB +154 B (+0.04%)
ℹ️ View Unchanged
Filename Size
packages/kas/dist/es/index.js 38.2 kB
packages/kmath/dist/es/index.js 4.26 kB
packages/math-input/dist/es/index.js 80.1 kB
packages/math-input/dist/es/strings.js 1.73 kB
packages/perseus-core/dist/es/index.js 906 B
packages/perseus-error/dist/es/index.js 866 B
packages/perseus-linter/dist/es/index.js 21.8 kB
packages/perseus/dist/es/strings.js 3.21 kB
packages/pure-markdown/dist/es/index.js 3.67 kB
packages/simple-markdown/dist/es/index.js 12.4 kB

compressed-size-action

Copy link

codecov bot commented Jun 28, 2024

Codecov Report

Attention: Patch coverage is 84.61538% with 12 lines in your changes missing coverage. Please review.

Project coverage is 70.86%. Comparing base (94067d7) to head (8578b70).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1383      +/-   ##
==========================================
+ Coverage   69.56%   70.86%   +1.29%     
==========================================
  Files         493      498       +5     
  Lines      104305   104465     +160     
  Branches     5293    10617    +5324     
==========================================
+ Hits        72563    74032    +1469     
+ Misses      31626    30433    -1193     
+ Partials      116        0     -116     

Impacted file tree graph

Files Coverage Δ
packages/perseus/src/index.ts 100.00% <100.00%> (ø)
...widgets/__testdata__/interactive-graph.testdata.ts 97.87% <ø> (ø)
.../widgets/interactive-graphs/graph-locked-layer.tsx 90.78% <100.00%> (+1.08%) ⬆️
...ctive-graphs/interactive-graph-question-builder.ts 98.55% <100.00%> (-1.45%) ⬇️
...eractive-graphs/locked-figures/locked-function.tsx 100.00% <100.00%> (ø)
...ts/graph-locked-figures/locked-figures-section.tsx 97.65% <16.66%> (-2.35%) ⬇️
packages/perseus-editor/src/components/util.ts 67.15% <30.00%> (-2.93%) ⬇️

... and 127 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 94067d7...8578b70. Read the comment docs.

@mark-fitzgerald mark-fitzgerald marked this pull request as ready for review July 1, 2024 22:36
@khan-actions-bot
Copy link
Contributor

khan-actions-bot commented Jul 1, 2024

Gerald

Required Reviewers
  • @Khan/perseus for changes to .changeset/brave-steaks-collect.md, packages/perseus/src/index.ts, packages/perseus/src/perseus-types.ts, packages/perseus-editor/src/components/util.ts, packages/perseus/src/widgets/__stories__/locked-functions.stories.tsx, packages/perseus/src/widgets/__testdata__/interactive-graph.testdata.ts, packages/perseus/src/widgets/__tests__/interactive-graph.test.ts, packages/perseus/src/widgets/interactive-graphs/graph-locked-layer.tsx, packages/perseus/src/widgets/interactive-graphs/interactive-graph-question-builder.test.ts, packages/perseus/src/widgets/interactive-graphs/interactive-graph-question-builder.ts, packages/perseus-editor/src/components/__tests__/util.test.ts, packages/perseus-editor/src/components/graph-locked-figures/locked-figures-section.tsx, packages/perseus/src/widgets/interactive-graphs/locked-figures/locked-function.tsx

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

Copy link
Contributor

@nishasy nishasy left a comment

Choose a reason for hiding this comment

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

THIS IS SO EXCITING! A few small comments, but nothing blocking.

Add comment to clarify difference between "equation" and "equationParsed".
Copy link
Contributor

github-actions bot commented Jul 1, 2024

npm Snapshot: Published

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

Example:

yarn add @khanacademy/perseus@PR1383

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

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

@mark-fitzgerald mark-fitzgerald merged commit 4b56e10 into main Jul 2, 2024
11 checks passed
@mark-fitzgerald mark-fitzgerald deleted the LEMS-1944-locked-figures-view-graphed-function branch July 2, 2024 19:00
color: LockedFigureColor;
strokeStyle: "solid" | "dashed";
equation: string; // This is the user-defined equation (as it was typed)
equationParsed?: {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@mark-fitzgerald The types in this file represent the "at rest" data representations for Perseus Data. As such, I try to avoid having keys that would never show up in Perseus JSON/data.

As I read this PR, this key would be created the first time this locked figure was rendered and then "cached" here for future renders, right?

My concern is folks thinking they can put data into this key in Perseus JSON and beginning to misuse the intent of this key (which I read as a memoization/cache of the parsed equation).

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, thanks for that clarification. I wasn't aware of this. I will refactor this accordingly when I implement the add/edit capability.

style: strokeStyle,
domain,
};
const equation = props.equationParsed || KAS.parse(props.equation).expr;
Copy link
Collaborator

Choose a reason for hiding this comment

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

When is equationParsed populated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was initially thinking that it would be populated when the content editor types the equation. I'm not sure how that would persist, though, so I'm rethinking the timing/management of this.

SonicScrewdriver added a commit that referenced this pull request Jul 8, 2024
This PR was opened by the [Changesets
release](https://github.com/changesets/action) GitHub action. When
you're ready to do a release, you can merge this and the packages will
be published to npm automatically. If you're not ready to do a release
yet, that's fine, whenever you add more changesets to main, this PR will
be updated.


# Releases
## @khanacademy/math-input@19.2.0

### Minor Changes

- [#1384](#1384)
[`5de483386`](5de4833)
Thanks [@catandthemachines](https://github.com/catandthemachines)! -
Updating TabBar experience in to use arrow-key navigation to access the
other TabItems. This will ensure the Expression Widget in perseus has
proper keyboard navigation for users.

## @khanacademy/perseus@24.3.0

### Minor Changes

- [#1383](#1383)
[`4b56e10de`](4b56e10)
Thanks [@mark-fitzgerald](https://github.com/mark-fitzgerald)! - View
Locked Functions in the Interactive Graph


- [#1392](#1392)
[`b710d07db`](b710d07)
Thanks [@SonicScrewdriver](https://github.com/SonicScrewdriver)! -
Creation of new angle graph for Mafs interactive graph widget

### Patch Changes

- [#1390](#1390)
[`7e6ccf38d`](7e6ccf3)
Thanks [@benchristel](https://github.com/benchristel)! - Internal: Move
graphing-agnostic, mathy functions in the interactive graph code to a
math/ folder.

- Updated dependencies
\[[`5de483386`](5de4833)]:
    -   @khanacademy/math-input@19.2.0

## @khanacademy/perseus-editor@7.0.3

### Patch Changes

- [#1383](#1383)
[`4b56e10de`](4b56e10)
Thanks [@mark-fitzgerald](https://github.com/mark-fitzgerald)! - View
Locked Functions in the Interactive Graph


- [#1390](#1390)
[`7e6ccf38d`](7e6ccf3)
Thanks [@benchristel](https://github.com/benchristel)! - Internal: Move
graphing-agnostic, mathy functions in the interactive graph code to a
math/ folder.


- [#1392](#1392)
[`b710d07db`](b710d07)
Thanks [@SonicScrewdriver](https://github.com/SonicScrewdriver)! -
Creation of new angle graph for Mafs interactive graph widget

- Updated dependencies
\[[`4b56e10de`](4b56e10),
[`7e6ccf38d`](7e6ccf3),
[`5de483386`](5de4833),
[`b710d07db`](b710d07)]:
    -   @khanacademy/perseus@24.3.0
    -   @khanacademy/math-input@19.2.0

## @khanacademy/perseus-dev-ui@1.6.1

### Patch Changes

- Updated dependencies
\[[`5de483386`](5de4833)]:
    -   @khanacademy/math-input@19.2.0
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.

4 participants