-
Notifications
You must be signed in to change notification settings - Fork 353
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] Open and Closing logic for unlimited polygon. #1852
base: main
Are you sure you want to change the base?
Conversation
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
Size Change: +471 B (+0.04%) Total Size: 1.29 MB
ℹ️ View Unchanged
|
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (95470bb) and published it to npm. You Example: yarn add @khanacademy/perseus@PR1852 If you are working in Khan Academy's webapp, you can run: ./dev/tools/bump_perseus_version.sh -t PR1852 |
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.
This is looking good! Thanks for backfilling tests for the other reducer actions.
I left some suggestions inline about things that I think could be refactored and simplified.
packages/perseus/src/widgets/interactive-graphs/reducer/interactive-graph-reducer.test.ts
Show resolved
Hide resolved
packages/perseus/src/widgets/interactive-graphs/reducer/interactive-graph-reducer.test.ts
Show resolved
Hide resolved
* Question: Should we always be setting the focus index to the end of the list? | ||
* Perhaps a better solution is the next point in the coordinates, | ||
* unless that does not exist. | ||
* */ |
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 this is a question for Caitlyn.
packages/perseus/src/widgets/interactive-graphs/reducer/interactive-graph-reducer.ts
Outdated
Show resolved
Hide resolved
packages/perseus/src/widgets/interactive-graphs/reducer/interactive-graph-reducer.test.ts
Outdated
Show resolved
Hide resolved
packages/perseus/src/widgets/interactive-graphs/reducer/interactive-graph-reducer.test.ts
Show resolved
Hide resolved
const shouldShowRemoveButton = | ||
showRemovePointButton && focusedPointIndex !== null; |
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 this variable now controls whether the remove button is enabled, I think it should be called shouldEnableRemoveButton
. It could also incorporate closedPolygon
:
const shouldShowRemoveButton = | |
showRemovePointButton && focusedPointIndex !== null; | |
const shouldEnableRemoveButton = | |
!closedPolygon && showRemovePointButton && focusedPointIndex !== null; |
I wonder if state.showRemovePointButton
should also be renamed to state.enableRemovePointButton
. Or if a different name entirely is appropriate, since that state variable doesn't completely control whether the button is enabled or not.
I'm kind of confused about when the remove button is enabled vs. disabled, and that might be a sign that it will be confusing to learners as well.
kind="secondary" | ||
color="destructive" | ||
// Disable button when polygon is closed. | ||
disabled={closedPolygon || !shouldShowRemoveButton} |
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.
Re: my comment above, maybe closedPolygon
should be incorporated into shouldShowRemoveButton
/shouldEnableRemoveButton
so all the logic can be in one place.
Summary:
Adding opening and closing behavior to unlimited polygon including:
Issue: LEMS-2570
Test plan:
yarn dev
and enableUnlimited Polygon
option in the Mafs dropdown. Or http://localhost:6006/?path=/story/perseus-widgets-interactive-graph--unlimited-polygon-with-mafs.Close polygon
button the close the polygon.Open polygon
button again to add or remove points.