Remove unlimited points via keyboard (#1570)

## Summary: https://github.com/user-attachments/assets/118cb3dc-4afe-4371-946b-f401b17c09e2 Remove unlimited points via keyboard. - A point can be removed by hitting delete - After a point is deleted the graph is focused Issue: https://khanacademy.atlassian.net/browse/LEMS-2280 ## Test plan: - Run the tests - Everything should pass Author: nicolecomputer Reviewers: benchristel, nicolecomputer, mark-fitzgerald Required Reviewers: Approved By: benchristel Checks: ✅ codecov/project, ✅ codecov/patch, ✅ Upload Coverage (ubuntu-latest, 20.x), ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Jest Coverage (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x), ✅ gerald Pull Request URL: #1570
Khan · Sep 3, 2024 · c4432ff · c4432ff
1 parent 78bb857
commit c4432ff
Show file tree

Hide file tree

Showing 11 changed files with 255 additions and 4 deletions.
diff --git a/.changeset/strange-crews-dress.md b/.changeset/strange-crews-dress.md
@@ -0,0 +1,5 @@
+---
+"@khanacademy/perseus": minor
+---
+
+Remove unlimited points via keyboard
diff --git a/packages/perseus/src/widgets/__tests__/interactive-graph.test.tsx b/packages/perseus/src/widgets/__tests__/interactive-graph.test.tsx
@@ -350,6 +350,7 @@ describe("tabbing forward on a Mafs segment graph", () => {
             flags: {mafs: {segment: true}},
         });
 
+        await userEvent.tab();
         await userEvent.tab();
 
         // eslint-disable-next-line testing-library/no-container, testing-library/no-node-access
@@ -359,11 +360,12 @@ describe("tabbing forward on a Mafs segment graph", () => {
         expect(movablePoints[0]).toHaveFocus();
     });
 
-    it("focuses the whole segment second", async () => {
+    it("focuses the whole segment third", async () => {
         const {container} = renderQuestion(segmentQuestion, {
             flags: {mafs: {segment: true}},
         });
 
+        await userEvent.tab();
         await userEvent.tab();
         await userEvent.tab();
 
@@ -382,6 +384,7 @@ describe("tabbing forward on a Mafs segment graph", () => {
         await userEvent.tab();
         await userEvent.tab();
         await userEvent.tab();
+        await userEvent.tab();
 
         // eslint-disable-next-line testing-library/no-container, testing-library/no-node-access
         const movablePoints = container.querySelectorAll(
@@ -407,6 +410,7 @@ describe("tabbing backward on a Mafs segment graph", () => {
         await userEvent.tab();
         await userEvent.tab();
         await userEvent.tab();
+        await userEvent.tab();
         await userEvent.tab({shift: true});
 
         // eslint-disable-next-line testing-library/no-container, testing-library/no-node-access
@@ -421,6 +425,7 @@ describe("tabbing backward on a Mafs segment graph", () => {
             flags: {mafs: {segment: true}},
         });
 
+        await userEvent.tab();
         await userEvent.tab();
         await userEvent.tab();
         await userEvent.tab({shift: true});

diff --git a/packages/perseus/src/widgets/interactive-graphs/graphs/point.tsx b/packages/perseus/src/widgets/interactive-graphs/graphs/point.tsx
@@ -79,6 +79,13 @@ export function UnlimitedPointGraph(props: PointGraphProps) {
                     onMove={(destination) =>
                         dispatch(actions.pointGraph.movePoint(i, destination))
                     }
+                    onFocusChange={(isFocused) => {
+                        if (isFocused) {
+                            dispatch(actions.pointGraph.focusPoint(i));
+                        } else {
+                            dispatch(actions.pointGraph.blurPoint());
+                        }
+                    }}
                 />
             ))}
         </>

diff --git a/packages/perseus/src/widgets/interactive-graphs/mafs-graph.test.tsx b/packages/perseus/src/widgets/interactive-graphs/mafs-graph.test.tsx
@@ -443,6 +443,7 @@ describe("MafsGraph", () => {
         const state: InteractiveGraphState = {
             type: "point",
             numPoints: 2,
+            focusedPointIndex: null,
             hasBeenInteractedWith: true,
             range: [
                 [-10, 10],
@@ -698,6 +699,7 @@ describe("MafsGraph", () => {
             const state: InteractiveGraphState = {
                 type: "point",
                 numPoints: "unlimited",
+                focusedPointIndex: null,
                 hasBeenInteractedWith: true,
                 range: [
                     [-10, 10],
@@ -731,6 +733,7 @@ describe("MafsGraph", () => {
             const state: InteractiveGraphState = {
                 type: "point",
                 numPoints: "unlimited",
+                focusedPointIndex: null,
                 hasBeenInteractedWith: true,
                 range: [
                     [-10, 10],

diff --git a/packages/perseus/src/widgets/interactive-graphs/mafs-graph.tsx b/packages/perseus/src/widgets/interactive-graphs/mafs-graph.tsx
@@ -57,6 +57,9 @@ export const MafsGraph = (props: MafsGraphProps) => {
     const {state, dispatch, labels, readOnly} = props;
     const [width, height] = props.box;
     const tickStep = props.step as vec.Vector2;
+
+    const graphRef = React.useRef<HTMLElement>(null);
+
     return (
         <GraphConfigContext.Provider
             value={{
@@ -77,16 +80,24 @@ export const MafsGraph = (props: MafsGraphProps) => {
                 <View
                     className="mafs-graph"
                     style={{
-                        height,
-                        width: "intrinsic",
                         position: "relative",
                         padding: "25px 25px 0 0",
                         boxSizing: "content-box",
                         marginLeft: "20px",
                         marginBottom: "20px",
                         pointerEvents: props.static ? "none" : "auto",
                         userSelect: "none",
+                        width,
+                        height,
+                    }}
+                    onKeyUp={(event) => {
+                        if (event.key === "Backspace") {
+                            dispatch(actions.global.deleteIntent());
+                            graphRef.current?.focus();
+                        }
                     }}
+                    ref={graphRef}
+                    tabIndex={0}
                 >
                     <LegacyGrid
                         box={props.box}

diff --git a/packages/perseus/src/widgets/interactive-graphs/reducer/initialize-graph-state.ts b/packages/perseus/src/widgets/interactive-graphs/reducer/initialize-graph-state.ts
@@ -77,6 +77,7 @@ export function initializeGraphState(
                 type: graph.type,
                 coords: getPointCoords(graph, range, step),
                 numPoints: graph.numPoints || 0,
+                focusedPointIndex: null,
             };
         case "circle":
             return {

diff --git a/packages/perseus/src/widgets/interactive-graphs/reducer/interactive-graph-action.ts b/packages/perseus/src/widgets/interactive-graphs/reducer/interactive-graph-action.ts
@@ -12,9 +12,15 @@ export type InteractiveGraphAction =
     | ChangeSnapStep
     | ChangeRange
     | AddPoint
-    | RemovePoint;
+    | RemovePoint
+    | FocusPoint
+    | BlurPoint
+    | DeleteIntent;
 
 export const actions = {
+    global: {
+        deleteIntent,
+    },
     angle: {
         movePoint,
     },
@@ -35,6 +41,8 @@ export const actions = {
         movePoint,
         addPoint,
         removePoint,
+        focusPoint,
+        blurPoint,
     },
     polygon: {
         movePoint,
@@ -57,6 +65,16 @@ export const actions = {
     },
 };
 
+export const DELETE_INTENT = "delete-intent";
+export interface DeleteIntent {
+    type: typeof DELETE_INTENT;
+}
+function deleteIntent(): DeleteIntent {
+    return {
+        type: DELETE_INTENT,
+    };
+}
+
 export const MOVE_LINE = "move-line";
 export interface MoveLine {
     type: typeof MOVE_LINE;
@@ -95,6 +113,28 @@ function removePoint(index: number): RemovePoint {
     };
 }
 
+export const FOCUS_POINT = "focus-point";
+export interface FocusPoint {
+    type: typeof FOCUS_POINT;
+    index: number;
+}
+function focusPoint(index: number): FocusPoint {
+    return {
+        type: FOCUS_POINT,
+        index,
+    };
+}
+
+export const BLUR_POINT = "blur-point";
+export interface BlurPoint {
+    type: typeof BLUR_POINT;
+}
+function blurPoint(): BlurPoint {
+    return {
+        type: BLUR_POINT,
+    };
+}
+
 export const MOVE_ALL = "move-all";
 export interface MoveAll {
     type: typeof MOVE_ALL;

diff --git a/packages/perseus/src/widgets/interactive-graphs/reducer/interactive-graph-reducer.test.ts b/packages/perseus/src/widgets/interactive-graphs/reducer/interactive-graph-reducer.test.ts
@@ -30,6 +30,7 @@ const basePointGraphState: InteractiveGraphState = {
         [-10, 10],
         [-10, 10],
     ],
+    focusedPointIndex: null,
     snapStep: [1, 1],
     coords: [],
 };
@@ -42,6 +43,7 @@ const baseUnlimitedPointGraphState: PointGraphState = {
         [-10, 10],
         [-10, 10],
     ],
+    focusedPointIndex: null,
     snapStep: [1, 1],
     coords: [],
 };
@@ -1015,3 +1017,116 @@ describe("unlimited points", () => {
         ]);
     });
 });
+
+describe("delete intent", () => {
+    it("does nothing when no points are selected", () => {
+        let state: PointGraphState = {
+            ...baseUnlimitedPointGraphState,
+        };
+
+        state = interactiveGraphReducer(
+            state,
+            actions.pointGraph.addPoint([1, 1]),
+        ) as PointGraphState;
+
+        state = interactiveGraphReducer(
+            state,
+            actions.pointGraph.addPoint([2, 2]),
+        ) as PointGraphState;
+
+        state = interactiveGraphReducer(
+            state,
+            actions.global.deleteIntent(),
+        ) as PointGraphState;
+
+        expect(state.coords).toMatchObject([
+            [1, 1],
+            [2, 2],
+        ]);
+    });
+
+    it("removes points when a point is focused and a delete intent is received", () => {
+        let state: PointGraphState = {
+            ...baseUnlimitedPointGraphState,
+        };
+
+        // Add some points
+        state = interactiveGraphReducer(
+            state,
+            actions.pointGraph.addPoint([1, 1]),
+        ) as PointGraphState;
+
+        state = interactiveGraphReducer(
+            state,
+            actions.pointGraph.addPoint([2, 2]),
+        ) as PointGraphState;
+
+        state = interactiveGraphReducer(
+            state,
+            actions.pointGraph.addPoint([3, 3]),
+        ) as PointGraphState;
+
+        // Focus a point
+        state = interactiveGraphReducer(
+            state,
+            actions.pointGraph.focusPoint(0),
+        ) as PointGraphState;
+
+        // Fire a delete intent
+        state = interactiveGraphReducer(
+            state,
+            actions.global.deleteIntent(),
+        ) as PointGraphState;
+
+        expect(state.coords).toMatchObject([
+            [2, 2],
+            [3, 3],
+        ]);
+    });
+});
+
+describe("unlimited points", () => {
+    it("adds points", () => {
+        const state: PointGraphState = {
+            ...baseUnlimitedPointGraphState,
+        };
+
+        const stateAfterAddingPoint = interactiveGraphReducer(
+            state,
+            actions.pointGraph.addPoint([8, 10]),
+        ) as PointGraphState;
+
+        expect(stateAfterAddingPoint.coords).toMatchObject([[8, 10]]);
+    });
+
+    it("removes points", () => {
+        let state: PointGraphState = {
+            ...baseUnlimitedPointGraphState,
+        };
+
+        state = interactiveGraphReducer(
+            state,
+            actions.pointGraph.addPoint([1, 1]),
+        ) as PointGraphState;
+
+        state = interactiveGraphReducer(
+            state,
+            actions.pointGraph.addPoint([2, 2]),
+        ) as PointGraphState;
+
+        state = interactiveGraphReducer(
+            state,
+            actions.pointGraph.addPoint([3, 3]),
+        ) as PointGraphState;
+
+        state = interactiveGraphReducer(
+            state,
+            actions.pointGraph.removePoint(1),
+        ) as PointGraphState;
+
+        expect(state.coords).toMatchObject([
+            [1, 1],
+            [3, 3],
+        ]);
+    });
+});