-
Notifications
You must be signed in to change notification settings - Fork 350
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
Add unlimited points to graph #1529
Conversation
Size Change: +796 B (+0.09%) Total Size: 857 kB
ℹ️ View Unchanged
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1529 +/- ##
==========================================
+ Coverage 70.00% 70.67% +0.66%
==========================================
Files 515 518 +3
Lines 106417 106622 +205
Branches 7705 11546 +3841
==========================================
+ Hits 74494 75350 +856
+ Misses 31744 31272 -472
+ Partials 179 0 -179
... and 140 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's really cool to see this coming together! The behavior seems solid; I left some suggestions for structural improvements inline.
packages/perseus/src/widgets/interactive-graphs/reducer/interactive-graph-reducer.ts
Outdated
Show resolved
Hide resolved
@@ -52,6 +52,7 @@ export interface LinearSystemGraphState extends InteractiveGraphStateCommon { | |||
export interface PointGraphState extends InteractiveGraphStateCommon { | |||
type: "point"; | |||
coords: Coord[]; | |||
numPoints?: number | "unlimited"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the number of points is given by coords.length
for graphs with fixed numbers of points, could this property just be a boolean? E.g. unlimitedPoints: boolean
?
In any case, I think this property shouldn't be optional, since it's always set in initializeGraphState
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd prefer to keep numPoints. It matches the state we receive and I don't think there are going to be any more places where it would get tricky.
packages/perseus/src/widgets/interactive-graphs/graphs/point.tsx
Outdated
Show resolved
Hide resolved
packages/perseus/src/widgets/interactive-graphs/graphs/point.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of the code in this file is now only used in the unlimited points case. That makes me think that maybe the unlimited point graph should be a separate component. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I broke them out into two components. I think it's easier to read now
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (088f82d) and published it to npm. You Example: yarn add @khanacademy/perseus@PR1529 If you are working in Khan Academy's webapp, you can run: ./dev/tools/bump_perseus_version.sh -t PR1529 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates! LGTM.
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/[email protected] ### Major Changes - [#1546](#1546) [`6cbe4947e`](6cbe494) Thanks [@SonicScrewdriver](https://github.com/SonicScrewdriver)! - Release PhET widget This PR releases a new PhET simulation widget to Perseus that requires an update in order to allow the support of new, upcoming content. Older versions of Perseus will be unable to render content that contains this widget. PhET simulations come from <https://phet.colorado.edu/>. ### Minor Changes - [#1529](#1529) [`0bf2711c0`](0bf2711) Thanks [@nicolecomputer](https://github.com/nicolecomputer)! - Adds unlimited point graph - [#1542](#1542) [`a7fc2a3e3`](a7fc2a3) Thanks [@aemandine](https://github.com/aemandine)! - Design update for PhET widget - [#1512](#1512) [`4f24be79d`](4f24be7) Thanks [@aemandine](https://github.com/aemandine)! - Add PhET widget - [#1532](#1532) [`6e102f9c4`](6e102f9) Thanks [@aemandine](https://github.com/aemandine)! - Add a content editor for the PhET widget - [#1533](#1533) [`cc1995daf`](cc1995d) Thanks [@nishasy](https://github.com/nishasy)! - [Locked labels] View locked labels in an Interactive Graph ### Patch Changes - [#1539](#1539) [`7805626e1`](7805626) Thanks [@nishasy](https://github.com/nishasy)! - [Locked Labels] Implement adding/editing/deleting a standalone locked label - [#1541](#1541) [`36471197c`](3647119) Thanks [@nicolecomputer](https://github.com/nicolecomputer)! - Adds a finite point question to dev gallery ## @khanacademy/[email protected] ### Major Changes - [#1546](#1546) [`6cbe4947e`](6cbe494) Thanks [@SonicScrewdriver](https://github.com/SonicScrewdriver)! - Release PhET widget This PR releases a new PhET simulation widget to Perseus that requires an update in order to allow the support of new, upcoming content. Older versions of Perseus will be unable to render content that contains this widget. PhET simulations come from <https://phet.colorado.edu/>. ### Minor Changes - [#1539](#1539) [`7805626e1`](7805626) Thanks [@nishasy](https://github.com/nishasy)! - [Locked Labels] Implement adding/editing/deleting a standalone locked label - [#1542](#1542) [`a7fc2a3e3`](a7fc2a3) Thanks [@aemandine](https://github.com/aemandine)! - Design update for PhET widget - [#1532](#1532) [`6e102f9c4`](6e102f9) Thanks [@aemandine](https://github.com/aemandine)! - Add a content editor for the PhET widget - [#1533](#1533) [`cc1995daf`](cc1995d) Thanks [@nishasy](https://github.com/nishasy)! - [Locked labels] View locked labels in an Interactive Graph ### Patch Changes - Updated dependencies \[[`7805626e1`](7805626), [`0bf2711c0`](0bf2711), [`a7fc2a3e3`](a7fc2a3), [`4f24be79d`](4f24be7), [`6cbe4947e`](6cbe494), [`36471197c`](3647119), [`6e102f9c4`](6e102f9), [`cc1995daf`](cc1995d)]: - @khanacademy/[email protected] ## @khanacademy/[email protected] ### Patch Changes - [#1541](#1541) [`36471197c`](3647119) Thanks [@nicolecomputer](https://github.com/nicolecomputer)! - Adds a finite point question to dev gallery Author: khan-actions-bot Reviewers: aemandine, #perseus Required Reviewers: Approved By: aemandine Checks: ✅ Upload Coverage (ubuntu-latest, 20.x), ⏭️ Publish npm snapshot, ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Jest Coverage (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ gerald Pull Request URL: #1544
Summary:
Allows adding an unlimited number of points to a graph.
unlimited.points.done.mov
Issue: LEMS-1816
Test plan: