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

[Interactive Graph] Update the builder with all currently migrated graph types #1373

Merged
merged 3 commits into from
Jun 24, 2024

Conversation

nishasy
Copy link
Contributor

@nishasy nishasy commented Jun 22, 2024

Summary:

I updated the builder utility with all currently migrated graph types. This
meant adding linear, linear system, ray, quadratic, and sinusoid.

I also added the ability to give them all starting coords. This is necessary
to prove that the starting coords can be set at all.

Now that it's clear they can be set, we can move onto adding the ability
to edit these coordinates in the editor UI in the next task.

NOTE: The task also includes start coords for the Points graph type, but
that doesn't seem to have the mafs flag on in storybook yet, so I didn't
include that here.

Issue: https://khanacademy.atlassian.net/browse/LEMS-2051

Test plan:

Storybook

Storybook previews showing custom start coords

segment segments linear
Screenshot 2024-06-21 at 5 08 25 PM Screenshot 2024-06-21 at 5 08 31 PM Screenshot 2024-06-21 at 5 08 36 PM
linear system ray circle
Screenshot 2024-06-21 at 5 08 40 PM Screenshot 2024-06-21 at 5 08 44 PM Screenshot 2024-06-21 at 5 08 49 PM
quadratic sinusoid
Screenshot 2024-06-21 at 5 08 53 PM Screenshot 2024-06-21 at 5 08 58 PM

@nishasy nishasy self-assigned this Jun 22, 2024
Copy link
Contributor

github-actions bot commented Jun 22, 2024

Size Change: -833 B (-0.1%)

Total Size: 845 kB

Filename Size Change
packages/perseus/dist/es/index.js 407 kB -833 B (-0.2%)
ℹ️ 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-editor/dist/es/index.js 272 kB
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

@nishasy nishasy force-pushed the view-graph-start-coords branch from f0d1856 to 37fe1c4 Compare June 22, 2024 00:22
@nishasy nishasy requested a review from benchristel June 22, 2024 00:23
@nishasy nishasy marked this pull request as ready for review June 22, 2024 00:24
@khan-actions-bot
Copy link
Contributor

khan-actions-bot commented Jun 22, 2024

Gerald

Required Reviewers
  • @Khan/perseus for changes to .changeset/short-badgers-wonder.md, packages/perseus-editor/src/__stories__/editor-page.stories.tsx, packages/perseus/src/widgets/__stories__/interactive-graph-regression.stories.tsx, packages/perseus/src/widgets/__testdata__/interactive-graph.testdata.ts, packages/perseus/src/widgets/interactive-graphs/interactive-graph-question-builder.test.ts, packages/perseus/src/widgets/interactive-graphs/interactive-graph-question-builder.ts

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

@khan-actions-bot khan-actions-bot requested a review from a team June 22, 2024 00:24
Copy link

codecov bot commented Jun 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.92%. Comparing base (8cbfeba) to head (4389efc).
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1373      +/-   ##
==========================================
+ Coverage   69.68%   70.92%   +1.24%     
==========================================
  Files         492      495       +3     
  Lines      103871   103927      +56     
  Branches     7505    10556    +3051     
==========================================
+ Hits        72382    73710    +1328     
+ Misses      31310    30217    -1093     
+ Partials      179        0     -179     

Impacted file tree graph

Files Coverage Δ
...widgets/__testdata__/interactive-graph.testdata.ts 100.00% <100.00%> (ø)
...ctive-graphs/interactive-graph-question-builder.ts 100.00% <100.00%> (ø)

... and 147 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 8cbfeba...4389efc. Read the comment docs.

Copy link
Member

@benchristel benchristel left a comment

Choose a reason for hiding this comment

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

Looks great, thank you! My one question is whether we want to let callers of the builder pass both startCoords and numSegments to the segment graph. In the common case, the information in numSegments is redundant and could be derived from startCoords. I could maybe see an argument that we'd want to let numSegments be different from startCoords in order to test what happens when the data is inconsistent. That's such an edge case, though, that I don't think it's worth complicating the builder for the sake of the O(1) tests we might write that take advantage of it.

I'll leave the decision up to you, though.

@nishasy
Copy link
Contributor Author

nishasy commented Jun 24, 2024

@benchristel I think it's still nice to have numSegments because it's accurate to the experience when coords isn't used. It would be nice to use the builder to create the segment graph with the default coords for a number of segments rather than having to find out and type all the coords out.

Copy link
Contributor

github-actions bot commented Jun 24, 2024

npm Snapshot: Published

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

Example:

yarn add @khanacademy/perseus@PR1373

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

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

@benchristel
Copy link
Member

It would be nice to use the builder to create the segment graph with the default coords for a number of segments rather than having to find out and type all the coords out.

I agree. I guess the way I'd prefer to accomplish that is by having a separate method to set non-default coords, so you could do either:

const question = interactiveGraphBuilder()
  .withNumSegments(3)
  .build();

or

const question = interactiveGraphBuilder()
  .withSegments([
    // ...
  ])
  .build();

@nishasy nishasy merged commit 9615106 into main Jun 24, 2024
13 checks passed
@nishasy nishasy deleted the view-graph-start-coords branch June 24, 2024 18:30
anakaren-rojas added a commit that referenced this pull request Jun 25, 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/perseus@24.0.0

### Major Changes

- [#1371](#1371)
[`ba5f33460`](ba5f334)
Thanks [@anakaren-rojas](https://github.com/anakaren-rojas)! - update
attributes for expression widget button

### Patch Changes

- [#1364](#1364)
[`35651e097`](35651e0)
Thanks [@Myranae](https://github.com/Myranae)! - Fix a bug in the
exercise editor where the preview did not update after a change to the
number of sides in polygon graphs


- [#1373](#1373)
[`961510673`](9615106)
Thanks [@nishasy](https://github.com/nishasy)! - [Interactive Graph]
Update the interactive graph builder with all currently migrated graph
types

## @khanacademy/perseus-editor@7.0.0

### Major Changes

- [#1371](#1371)
[`ba5f33460`](ba5f334)
Thanks [@anakaren-rojas](https://github.com/anakaren-rojas)! - update
attributes for expression widget button

### Patch Changes

- [#1373](#1373)
[`961510673`](9615106)
Thanks [@nishasy](https://github.com/nishasy)! - [Interactive Graph]
Update the interactive graph builder with all currently migrated graph
types

- Updated dependencies
\[[`35651e097`](35651e0),
[`961510673`](9615106),
[`ba5f33460`](ba5f334)]:
    -   @khanacademy/perseus@24.0.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.

3 participants