Create UI for adding a locked point to interactive graph editor (mafs only) #1074

nishasy · 2024-03-14T01:34:10Z

Summary:

UI for adding a locked point to interactive graph editor! This will only be for mafs graphs as mafs graphs have a slightly different look and API (and also it was just not working very well with graphie anyway).

This UI includes:

A dropdown that allows adding elements (just "Point" for now) added to the interactive graph editor.
Once added, settings for points
- The ability to delete the setting in question
- The x and y coordinates of the point
The removal of the locked point investigation from graph.tsx and related files.

The ability to change the color of a point is coming in a future PR.

Issue: LEMS-1813

Test plan:

yarn jest packages/perseus-editor/src/widgets/__tests__/interactive-graph-editor.test.tsx
yarn jest packages/perseus-editor/src/components/__tests__/util.test.ts

Storybook

Go to http://localhost:6006/?path=/story/perseus-editor-widgets-interactive-graph-editor--with-locked-points
Scroll down to the "Add an element" dropdown
Click "Add an element"
Click "Point"
Confirm that a black point appears at (0, 0) on the graph
Change the coordinates and confirm that the point moves
Add another point
Use the trash button to delete a point
Confirm that the correct point is deleted

Demo:

Screen.Recording.2024-03-13.at.7.28.37.PM.mov

todo Issue: https://khanacademy.atlassian.net/browse/LEMS-1702 Test plan: todo

github-actions · 2024-03-14T01:36:45Z

Size Change: +1.73 kB (0%)

Total Size: 812 kB

Filename	Size	Change
`packages/perseus-editor/dist/es/index.js`	265 kB	+1.91 kB (+1%)
`packages/perseus/dist/es/index.js`	385 kB	-177 B (0%)

ℹ️ View Unchanged

Filename	Size
`packages/kas/dist/es/index.js`	38.1 kB
`packages/kmath/dist/es/index.js`	4.27 kB
`packages/math-input/dist/es/index.js`	80.4 kB
`packages/perseus-core/dist/es/index.js`	908 B
`packages/perseus-error/dist/es/index.js`	878 B
`packages/perseus-linter/dist/es/index.js`	21.8 kB
`packages/pure-markdown/dist/es/index.js`	3.69 kB
`packages/simple-markdown/dist/es/index.js`	12.4 kB

_{compressed-size-action}

codecov · 2024-03-14T01:55:13Z

Codecov Report

Attention: Patch coverage is 98.86686% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 68.68%. Comparing base (6196375) to head (688c291).
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1074      +/-   ##
==========================================
+ Coverage   67.28%   68.68%   +1.39%     
==========================================
  Files         436      442       +6     
  Lines       96519    96885     +366     
  Branches     6720    10281    +3561     
==========================================
+ Hits        64946    66543    +1597     
+ Misses      31573    30342    -1231

Files	Coverage Δ
...ages/perseus-editor/src/components/labeled-row.tsx	`98.07% <100.00%> (+0.16%)`	⬆️
...eus-editor/src/components/locked-figure-select.tsx	`100.00% <100.00%> (ø)`
...s-editor/src/components/locked-figures-section.tsx	`100.00% <100.00%> (ø)`
...us-editor/src/components/locked-point-settings.tsx	`100.00% <100.00%> (ø)`
packages/perseus-editor/src/components/util.ts	`40.32% <100.00%> (+37.69%)`	⬆️
packages/perseus/src/components/graph.tsx	`79.36% <100.00%> (+17.92%)`	⬆️
packages/perseus/src/index.ts	`100.00% <100.00%> (ø)`
packages/perseus/src/widgets/interactive-graph.tsx	`54.69% <ø> (+12.87%)`	⬆️
...us-editor/src/widgets/interactive-graph-editor.tsx	`87.18% <96.00%> (-2.47%)`	⬇️
...s-editor/src/components/locked-figure-settings.tsx	`88.46% <88.46%> (ø)`

... and 101 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 6196375...688c291. Read the comment docs.

nishasy · 2024-03-14T01:41:18Z

packages/perseus-editor/src/components/locked-figure-select.tsx

+    return (
+        <View style={styles.container}>
+            <ActionMenu menuText="Add element" style={styles.addElementSelect}>
+                {[


Note: This needs the brackets because there's only one child. We can get rid of this after adding more types.

:til: I would have used a fragment here.

nishasy · 2024-03-14T02:16:50Z

packages/perseus-editor/src/widgets/__tests__/interactive-graph-editor.test.tsx

@@ -43,6 +61,9 @@ describe("InteractiveGraphEditor", () => {

        render(
            <InteractiveGraphEditor {...baseProps} onChange={onChangeMock} />,
+            {
+                wrapper: RenderStateRoot,
+            },


Needed to add RenderStateRoot because of the unique ID provider.

:til: about render()'s wrapper! Is it worth a render helper in this test file?

nishasy · 2024-03-14T02:31:54Z

packages/perseus-editor/src/components/labeled-row.tsx

The click target was taking up the whole row when it wasn't supposed to. Figured I'd fix it really quickly while I was here.

khan-actions-bot · 2024-03-14T02:36:09Z

Gerald

Required Reviewers

@Khan/perseus for changes to .changeset/short-eggs-rest.md, packages/perseus-editor/package.json, packages/perseus/src/index.ts, packages/perseus/src/perseus-types.ts, packages/perseus/src/components/graph.tsx, packages/perseus/src/widgets/interactive-graph.tsx, packages/perseus-editor/src/components/labeled-row.tsx, packages/perseus-editor/src/components/locked-figure-select.tsx, packages/perseus-editor/src/components/locked-figure-settings.tsx, packages/perseus-editor/src/components/locked-figures-section.tsx, packages/perseus-editor/src/components/locked-point-settings.tsx, packages/perseus-editor/src/components/util.ts, packages/perseus-editor/src/widgets/interactive-graph-editor.tsx, packages/perseus/src/components/__tests__/graph.test.tsx, packages/perseus-editor/src/components/__tests__/util.test.ts, packages/perseus-editor/src/widgets/__stories__/interactive-graph-editor.argtypes.ts, packages/perseus-editor/src/widgets/__stories__/interactive-graph-editor.stories.tsx, packages/perseus-editor/src/widgets/__tests__/interactive-graph-editor.test.tsx

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

nedredmond

This looks good to me! Thanks for all the tests!

jeremywiebe

Coooool. I left some comments, nothing that has to be done. :)

jeremywiebe · 2024-03-14T18:04:58Z

packages/perseus-editor/src/components/__tests__/util.test.ts

@@ -0,0 +1,42 @@
+import {getValidNumberFromString, getDefaultFigureForFigureType} from "../util";
+
+describe("getValidNumberFromString", () => {


Thanks for adding tests for this!

jeremywiebe · 2024-03-14T18:05:54Z

packages/perseus-editor/src/components/locked-figure-select.tsx

@@ -0,0 +1,49 @@
+/**
+ * Dropdown for selecting a locked figure to add to an interactive graph.
+ * Locked figures are elements (points, segmeents, etc.) that are not


Suggested change

* Locked figures are elements (points, segmeents, etc.) that are not

* Locked figures are elements (points, segments, etc.) that are not

jeremywiebe · 2024-03-14T18:11:48Z

packages/perseus-editor/src/components/locked-figure-select.tsx

+    return (
+        <View style={styles.container}>
+            <ActionMenu menuText="Add element" style={styles.addElementSelect}>
+                {[


:til: I would have used a fragment here.

jeremywiebe · 2024-03-14T18:16:41Z

packages/perseus-editor/src/components/locked-point-settings.tsx

+
+            {/* Coordinates */}
+            <View>
+                <View style={styles.row}>


thought: From a design perspective, I wonder if we should put the x and y coord inputs on the same row. Vertical space is so precious in our editors and so I think we should try to avoid scrolling if at all possible.

What do you think about something like this:

*Point* Coordinate [x ] [y ]

Where the inputs would have a watermark/placeholder for which axis they're for? I think authors using this editor would understand "x" always comes first so even with data in the fields they'd likely understand it's an x, y coordinate pair.

jeremywiebe · 2024-03-14T18:17:46Z

packages/perseus-editor/src/components/locked-figures-section.tsx

+                <LockedFigureSettings
+                    key={`${uniqueId}-locked-${figure}-${index}`}
+                    {...figure}
+                    onRemove={() => removeLockedFigure(index)}


I have a possibly-unfounded concern that using indexes for these callbacks could be problematic. I know for react, you aren't supposed to use array indexes for keys. Does that apply here?

jeremywiebe · 2024-03-14T18:20:52Z

packages/perseus-editor/src/widgets/__tests__/interactive-graph-editor.test.tsx

@@ -43,6 +61,9 @@ describe("InteractiveGraphEditor", () => {

        render(
            <InteractiveGraphEditor {...baseProps} onChange={onChangeMock} />,
+            {
+                wrapper: RenderStateRoot,
+            },


:til: about render()'s wrapper! Is it worth a render helper in this test file?

jeremywiebe · 2024-03-14T18:21:25Z

packages/perseus-editor/src/widgets/interactive-graph-editor.tsx

+     * Locked figures are graph elements (points, lines, line segmeents,
+     * etc.) that are locked in place and not interactive.
+     */
+    lockedFigures?: Array<LockedFigure>;


Suggested change

lockedFigures?: Array<LockedFigure>;

lockedFigures?: ReadonlyArray<LockedFigure>;

@benchristel

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-dev-ui@1.3.0 ### Minor Changes - [#1071](#1071) [`eb637b35`](eb637b3) Thanks [@benchristel](https://github.com/benchristel)! - Add a "check answer" button to interactive graphs displayed in the dev flipbook UI ## @khanacademy/perseus@20.2.0 ### Minor Changes - [#1067](#1067) [`6196375a`](6196375) Thanks [@nishasy](https://github.com/nishasy)! - Can pass a point into `lockedFigures` into MafsGraph and a point will be displayed. - [#1074](#1074) [`a263e940`](a263e94) Thanks [@nishasy](https://github.com/nishasy)! - Add "add a locked figure" UI to interactive graph editor + adding points (mafs graphs only) ### Patch Changes - [#1068](#1068) [`881da724`](881da72) Thanks [@benchristel](https://github.com/benchristel)! - Adjust styling of the movable points and line segments on the Mafs segment graph - [#1071](#1071) [`eb637b35`](eb637b3) Thanks [@benchristel](https://github.com/benchristel)! - Add a "check answer" button to interactive graphs displayed in the dev flipbook UI - [#1073](#1073) [`ea0db7d9`](ea0db7d) Thanks [@benchristel](https://github.com/benchristel)! - Fix color of Mafs line segments ## @khanacademy/perseus-editor@5.1.0 ### Minor Changes - [#1074](#1074) [`a263e940`](a263e94) Thanks [@nishasy](https://github.com/nishasy)! - Add "add a locked figure" UI to interactive graph editor + adding points (mafs graphs only) ### Patch Changes - Updated dependencies \[[`881da724`](881da72), [`eb637b35`](eb637b3), [`6196375a`](6196375), [`a263e940`](a263e94), [`ea0db7d9`](ea0db7d)]: - @khanacademy/perseus@20.2.0 Author: khan-actions-bot Reviewers: benchristel Required Reviewers: Approved By: benchristel Checks: ✅ codecov/project, ✅ codecov/patch, ✅ Upload Coverage, ✅ Jest Coverage (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ⏭️ Publish npm snapshot, ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Extract i18n strings (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), ✅ Jest Coverage (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ gerald, ✅ Prime node_modules cache for primary configuration (ubuntu-latest, 20.x) Pull Request URL: #1069

nishasy added 4 commits March 13, 2024 14:18

Create UI for adding a locked point to interactive graph editor

6108c7a

todo Issue: https://khanacademy.atlassian.net/browse/LEMS-1702 Test plan: todo

remove color

ec4fc26

cleanup

d1e396f

Add tests

8b42fca

nishasy self-assigned this Mar 14, 2024

Add changeset. Fix tests

8b72295

nishasy added 2 commits March 13, 2024 18:57

Add comments explaining what the new files do

def8a92

Add more tests

9f924aa

nishasy commented Mar 14, 2024

View reviewed changes

Another small util test

688c291

nishasy commented Mar 14, 2024

View reviewed changes

nishasy requested review from jeremywiebe, nedredmond, benchristel and mark-fitzgerald March 14, 2024 02:32

nishasy marked this pull request as ready for review March 14, 2024 02:35

khan-actions-bot requested a review from a team March 14, 2024 02:36

nedredmond approved these changes Mar 14, 2024

View reviewed changes

nishasy merged commit a263e94 into main Mar 14, 2024
13 checks passed

nishasy deleted the ige-add-point branch March 14, 2024 17:25

khan-actions-bot mentioned this pull request Mar 13, 2024

Version Packages #1069

Merged

jeremywiebe reviewed Mar 14, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create UI for adding a locked point to interactive graph editor (mafs only) #1074

Create UI for adding a locked point to interactive graph editor (mafs only) #1074

nishasy commented Mar 14, 2024 •

edited

Loading

github-actions bot commented Mar 14, 2024 •

edited

Loading

codecov bot commented Mar 14, 2024 •

edited

Loading

nishasy Mar 14, 2024

jeremywiebe Mar 14, 2024

nishasy Mar 14, 2024

jeremywiebe Mar 14, 2024

nishasy Mar 14, 2024

khan-actions-bot commented Mar 14, 2024

nedredmond left a comment

jeremywiebe left a comment

jeremywiebe Mar 14, 2024

jeremywiebe Mar 14, 2024

jeremywiebe Mar 14, 2024

jeremywiebe Mar 14, 2024

jeremywiebe Mar 14, 2024

jeremywiebe Mar 14, 2024

jeremywiebe Mar 14, 2024

		@@ -0,0 +1,42 @@
		import {getValidNumberFromString, getDefaultFigureForFigureType} from "../util";

		describe("getValidNumberFromString", () => {

	* Locked figures are elements (points, segmeents, etc.) that are not
	* Locked figures are elements (points, segments, etc.) that are not

	lockedFigures?: Array<LockedFigure>;
	lockedFigures?: ReadonlyArray<LockedFigure>;

Create UI for adding a locked point to interactive graph editor (mafs only) #1074

Create UI for adding a locked point to interactive graph editor (mafs only) #1074

Conversation

nishasy commented Mar 14, 2024 • edited Loading

Summary:

Test plan:

Demo:

github-actions bot commented Mar 14, 2024 • edited Loading

codecov bot commented Mar 14, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khan-actions-bot commented Mar 14, 2024

Gerald

nedredmond left a comment

Choose a reason for hiding this comment

jeremywiebe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nishasy commented Mar 14, 2024 •

edited

Loading

github-actions bot commented Mar 14, 2024 •

edited

Loading

codecov bot commented Mar 14, 2024 •

edited

Loading