From c4432ffad978a224b7d981e1577c7897342a01ee Mon Sep 17 00:00:00 2001 From: "Nicole W." <76132106+nicolecomputer@users.noreply.github.com> Date: Tue, 3 Sep 2024 12:46:44 -0700 Subject: [PATCH] Remove unlimited points via keyboard (#1570) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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: https://github.com/Khan/perseus/pull/1570 --- .changeset/strange-crews-dress.md | 5 + .../__tests__/interactive-graph.test.tsx | 7 +- .../interactive-graphs/graphs/point.tsx | 7 ++ .../interactive-graphs/mafs-graph.test.tsx | 3 + .../widgets/interactive-graphs/mafs-graph.tsx | 15 ++- .../reducer/initialize-graph-state.ts | 1 + .../reducer/interactive-graph-action.ts | 42 ++++++- .../reducer/interactive-graph-reducer.test.ts | 115 ++++++++++++++++++ .../reducer/interactive-graph-reducer.ts | 62 ++++++++++ .../stateful-mafs-graph.test.tsx | 1 + .../src/widgets/interactive-graphs/types.ts | 1 + 11 files changed, 255 insertions(+), 4 deletions(-) create mode 100644 .changeset/strange-crews-dress.md diff --git a/.changeset/strange-crews-dress.md b/.changeset/strange-crews-dress.md new file mode 100644 index 0000000000..3eacb99e0b --- /dev/null +++ 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 index 53a861a01a..0182557004 100644 --- 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 index 1420b45826..193515a4de 100644 --- 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 index 965e164365..d26b50a34f 100644 --- 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 index 0f9d1e987a..15bbaac3b7 100644 --- 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(null); + return ( { { 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} > { ]); }); }); + +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], + ]); + }); +}); diff --git a/packages/perseus/src/widgets/interactive-graphs/reducer/interactive-graph-reducer.ts b/packages/perseus/src/widgets/interactive-graphs/reducer/interactive-graph-reducer.ts index 6d3e69ced1..b5069e619c 100644 --- a/packages/perseus/src/widgets/interactive-graphs/reducer/interactive-graph-reducer.ts +++ b/packages/perseus/src/widgets/interactive-graphs/reducer/interactive-graph-reducer.ts @@ -20,6 +20,7 @@ import {bound} from "../utils"; import {initializeGraphState} from "./initialize-graph-state"; import { + actions, CHANGE_RANGE, CHANGE_SNAP_STEP, type ChangeRange, @@ -42,6 +43,12 @@ import { type AddPoint, REMOVE_POINT, type RemovePoint, + DELETE_INTENT, + type DeleteIntent, + FOCUS_POINT, + type FocusPoint, + BLUR_POINT, + type BlurPoint, } from "./interactive-graph-action"; import type {Coord} from "../../../interactive2/types"; @@ -82,11 +89,65 @@ export function interactiveGraphReducer( return doAddPoint(state, action); case REMOVE_POINT: return doRemovePoint(state, action); + case FOCUS_POINT: + return doFocusPoint(state, action); + case BLUR_POINT: + return doBlurPoint(state, action); + case DELETE_INTENT: + return doDeleteIntent(state, action); default: throw new UnreachableCaseError(action); } } +function doDeleteIntent( + state: InteractiveGraphState, + action: DeleteIntent, +): InteractiveGraphState { + // For unlimited point graphs + if (state.type === "point" && state.numPoints === "unlimited") { + // if there's a focused point + if (state.focusedPointIndex !== null) { + // Remove the focused focus + return doRemovePoint( + state, + actions.pointGraph.removePoint(state.focusedPointIndex), + ); + } + } + return state; +} + +function doFocusPoint( + state: InteractiveGraphState, + action: FocusPoint, +): InteractiveGraphState { + switch (state.type) { + case "point": + return { + ...state, + focusedPointIndex: action.index, + }; + default: + return state; + } +} + +function doBlurPoint( + state: InteractiveGraphState, + action: BlurPoint, +): InteractiveGraphState { + switch (state.type) { + case "point": + return { + ...state, + focusedPointIndex: null, + }; + default: + return state; + } +} + function doMovePointInFigure( state: InteractiveGraphState, action: MovePointInFigure, @@ -543,6 +604,7 @@ function doRemovePoint( return { ...state, coords: state.coords.filter((_, i) => i !== action.index), + focusedPointIndex: null, }; } diff --git a/packages/perseus/src/widgets/interactive-graphs/stateful-mafs-graph.test.tsx b/packages/perseus/src/widgets/interactive-graphs/stateful-mafs-graph.test.tsx index 4e7a334b1e..83fbd81f7b 100644 --- a/packages/perseus/src/widgets/interactive-graphs/stateful-mafs-graph.test.tsx +++ b/packages/perseus/src/widgets/interactive-graphs/stateful-mafs-graph.test.tsx @@ -67,6 +67,7 @@ describe("StatefulMafsGraph", () => { />, ); + await userEvent.tab(); await userEvent.tab(); await userEvent.keyboard("{arrowup}"); diff --git a/packages/perseus/src/widgets/interactive-graphs/types.ts b/packages/perseus/src/widgets/interactive-graphs/types.ts index 169f469cc1..5b503e7e0c 100644 --- a/packages/perseus/src/widgets/interactive-graphs/types.ts +++ b/packages/perseus/src/widgets/interactive-graphs/types.ts @@ -53,6 +53,7 @@ export interface PointGraphState extends InteractiveGraphStateCommon { type: "point"; coords: Coord[]; numPoints?: number | "unlimited"; + focusedPointIndex: number | null; } export interface RayGraphState extends InteractiveGraphStateCommon {