[Interactive Graph Editor] Add the ability to reorder locked figure settings #1360
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… only take degrees for the angle input We had it so that locked ellipse settings allow for either degrees or radians for the angel input. However, I realized that we're not saving the data for what unit is being used. This means that if an angle were saved as "30 degrees" and then the page were reloaded, it would then show up as "0.523599 radians". For the sake of simplicty and consistency, I'm changing this so that it only takes degrees, and I'm changing it to a number input since it no longer needs to evaluate expressions with pi (that required it being a text input). Issue: https://khanacademy.atlassian.net/browse/LEMS-1941 Test plan: `yarn jest` Storybook - http://localhost:6006/?path=/story/perseuseditor-editorpage--mafs-with-locked-figures-m-2-flag - Confirm that the ellipse settings angle input shows degrees only - Confirm that it is rotated correctly
…ettings Added some icon buttons so that content authors can reorder the locked figure settings. This can be used to make sure that one figure can render on top of another figure as desired. (The lower they are in the settings, the more in the front they are on the graph.) Issue: https://khanacademy.atlassian.net/browse/LEMS-1951 Test plan: todo
|
Size Change: +475 B (+0.06%) Total Size: 846 kB
ℹ️ View Unchanged
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1360 +/- ##
==========================================
+ Coverage 69.65% 70.78% +1.13%
==========================================
Files 489 495 +6
Lines 103415 103850 +435
Branches 5280 10515 +5235
==========================================
+ Hits 72033 73510 +1477
+ Misses 31266 30340 -926
+ Partials 116 0 -116
... and 141 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 |
benchristel
approved these changes
Jun 20, 2024
| export type Props = Omit< | ||
| LockedFigureSettingsCommonProps, | ||
| "onMove" | "onRemove" | ||
| > & |
There was a problem hiding this comment.
Minor: I'd prefer to list the props explicitly instead of using Omit:
export type Props = LockedPointType & {
expanded?: boolean;
onToggle?: (expanded: boolean) => void;
// ...
}When I'm trying to use a component, I want to know what props I can/must pass. When I'm working on code inside a component, I want to know what props are available. Having all the props listed together, in the component file makes both of these tasks easier. Yes, the types are duplicated with this approach, but the typechecker will tell us if the duplicated types ever get out of sync.
| <IconButton | ||
| icon={caretDoubleDownIcon} | ||
| size="small" | ||
| aria-label={`Move locked ${figureType} to the bottom`} |
There was a problem hiding this comment.
Minor: in graphics programs, the usual labels for these concepts are "bring to front" and "send to back". E.g. here's how Google Drawings does it:
"front"/"forward"/"back"/"backward" might be more recognizable than "top"/"up"/"down"/"bottom".
…anded state array updated with the movement.
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (769f454) and published it to npm. You Example: yarn add @khanacademy/perseus@PR1360If you are working in Khan Academy's webapp, you can run: ./dev/tools/bump_perseus_version.sh -t PR1360 |
|
The parent pull-request (#1357) has been merged into |
Merged
nishasy
added a commit
that referenced
this pull request
Jun 20, 2024
…1365) ## Summary: The action buttons that were added in #1360 need to be behind the m2 flag. Adding the check here. Issue: https://khanacademy.atlassian.net/browse/LEMS-1951 ## Test plan: Storybook - Go to http://localhost:6006/?path=/story/perseuseditor-editorpage--mafs-with-locked-figures-current - It should not have the action buttons in the locked figure settings - Go to http://localhost:6006/?path=/story/perseuseditor-editorpage--mafs-with-locked-figures-m-2-flag - Expand all the locked figure settings. - They should all have the action buttons. ### Current <img width="371" alt="Screenshot 2024-06-20 at 2 02 23 PM" src="https://github.com/Khan/perseus/assets/13231763/97e9cca9-c0db-4900-8360-7eb6e67ba2dd"> ### M2 flag <img width="369" alt="Screenshot 2024-06-20 at 2 02 39 PM" src="https://github.com/Khan/perseus/assets/13231763/101941c8-b4f5-450f-b748-eda8ac2e61fa"> <img width="370" alt="Screenshot 2024-06-20 at 2 02 46 PM" src="https://github.com/Khan/perseus/assets/13231763/820cce4a-c54b-4c89-a94e-0db3579c8425"> Author: nishasy Reviewers: mark-fitzgerald, benchristel Required Reviewers: Approved By: mark-fitzgerald Checks: ✅ gerald, ✅ Upload Coverage (ubuntu-latest, 20.x), ⏭️ Publish npm snapshot, ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Jest Coverage (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), 🚫 Upload Coverage, ✅ gerald, ⏭️ Publish npm snapshot, 🚫 Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), 🚫 Cypress (ubuntu-latest, 20.x), 🚫 Jest Coverage (ubuntu-latest, 20.x), 🚫 Check for .changeset entries for all changed files (ubuntu-latest, 20.x), 🚫 Check builds for changes in size (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x), ✅ gerald Pull Request URL: #1365
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
Added some icon buttons so that content authors can reorder the locked figure
settings. This can be used to make sure that one figure can render on top
of another figure as desired. (The lower they are in the settings, the more
in the front they are on the graph.)
Note: I added more tests to the
interactive-graph-editor-locked-figures.test.tsxfile.I think it's getting too long, so I'm considering breaking it up into a different file per
describeblock in the future in its own PR.Issue: https://khanacademy.atlassian.net/browse/LEMS-1951
Test plan:
yarn jest packages/perseus-editor/src/widgets/__tests__/interactive-graph-editor-locked-figures.test.tsxPlay around with the buttons in the storybook file, as demonstrated in the videos below.
http://localhost:6006/?path=/story/perseuseditor-editorpage--mafs-with-locked-figures-m-2-flag
Videos
Reordering in editor
Screen.Recording.2024-06-18.at.4.14.18.PM.mov
Updating order on graph
Screen.Recording.2024-06-18.at.3.07.08.PM.mov