From bfb7b671597ccf39952f32359d9874f09c9e2384 Mon Sep 17 00:00:00 2001 From: Nisha Yerunkar Date: Tue, 10 Dec 2024 17:36:24 -0800 Subject: [PATCH 1/8] [Polygon] Close polygon if last point is moved to overlap the first point --- .../src/__stories__/flags-for-api-options.ts | 2 ++ .../interactive-graph-editor.stories.tsx | 7 +++++++ .../reducer/interactive-graph-reducer.ts | 17 +++++++++++++++++ 3 files changed, 26 insertions(+) diff --git a/packages/perseus-editor/src/__stories__/flags-for-api-options.ts b/packages/perseus-editor/src/__stories__/flags-for-api-options.ts index c67bf16c7a..213ea5797d 100644 --- a/packages/perseus-editor/src/__stories__/flags-for-api-options.ts +++ b/packages/perseus-editor/src/__stories__/flags-for-api-options.ts @@ -13,6 +13,8 @@ export const flags = { ray: true, point: true, none: true, + "unlimited-point": true, + "unlimited-polygon": true, // Locked figures flags "interactive-graph-locked-features-labels": true, diff --git a/packages/perseus-editor/src/__stories__/interactive-graph-editor.stories.tsx b/packages/perseus-editor/src/__stories__/interactive-graph-editor.stories.tsx index 51137cfd45..b7dc16a04c 100644 --- a/packages/perseus-editor/src/__stories__/interactive-graph-editor.stories.tsx +++ b/packages/perseus-editor/src/__stories__/interactive-graph-editor.stories.tsx @@ -22,6 +22,7 @@ import { segmentWithStartingCoordsQuestion, segmentsWithStartingCoordsQuestion, sinusoidWithStartingCoordsQuestion, + unlimitedPolygonQuestion, } from "../../../perseus/src/widgets/interactive-graphs/interactive-graph.testdata"; import {registerAllWidgetsAndEditorsForTesting} from "../util/register-all-widgets-and-editors-for-testing"; @@ -125,6 +126,12 @@ export const InteractiveGraphPolygon = (): React.ReactElement => { ); }; +export const InteractiveGraphUnlimitedPolygon = (): React.ReactElement => { + return ( + + ); +}; + export const InteractiveGraphAngle = (): React.ReactElement => { return ( Date: Wed, 11 Dec 2024 12:06:00 -0800 Subject: [PATCH 2/8] Change the behavior so that it doesn't auto-close. The user has to click the button. --- .../reducer/interactive-graph-reducer.ts | 33 +++++++++---------- 1 file changed, 16 insertions(+), 17 deletions(-) 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 75d95b79bd..ca0e9220a3 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 @@ -200,6 +200,22 @@ function doClickPoint( function doClosePolygon(state: InteractiveGraphState): InteractiveGraphState { if (isUnlimitedGraphState(state) && state.type === "polygon") { + // If the last point is the same as the first point when the + // "Close shape" button is clicked, we should remove the + // last point before closing the polygon. Otherwise, it would + // be counted as an extra vertex and the question would be + // marked wrong. (Making this consistent with the behavior if + // the user had clicked on the first point to close the shape.) + const lastPoint = state.coords[state.coords.length - 1]; + const firstPoint = state.coords[0]; + if (_.isEqual(lastPoint, firstPoint)) { + return { + ...state, + coords: state.coords.slice(0, -1), + closedPolygon: true, + }; + } + return { ...state, closedPolygon: true, @@ -493,23 +509,6 @@ function doMovePoint( return state; } - // If the last point is being moved to the same position as - // the first point, close the polygon. - if ( - !state.closedPolygon && // not already close - action.index === newCoords.length - 1 && // last point - state.coords[0][0] === newValue[0] && // same x as first point - state.coords[0][1] === newValue[1] // same y as first point - ) { - // Remove the last point so it isn't a duplicate of the - // first point int he polygon representation. This is - // consistent with the cases in which the "Close shape" - // button is clicked or the first point is clicked to - // close the polygon. - newCoords.pop(); - state.closedPolygon = true; - } - return { ...state, hasBeenInteractedWith: true, From bdd170200f30fb652db536be729d46bbda44fb73 Mon Sep 17 00:00:00 2001 From: Nisha Yerunkar Date: Wed, 11 Dec 2024 16:53:16 -0800 Subject: [PATCH 3/8] fix issue with duped points --- .../interactive-graphs/graphs/polygon.tsx | 10 ++++++--- .../interactive-graphs/graphs/utils.test.ts | 22 ++++++++++++++++++- .../interactive-graphs/graphs/utils.ts | 19 ++++++++++++++++ .../widgets/interactive-graphs/mafs-graph.tsx | 4 +++- .../reducer/interactive-graph-reducer.ts | 18 +++------------ 5 files changed, 53 insertions(+), 20 deletions(-) diff --git a/packages/perseus/src/widgets/interactive-graphs/graphs/polygon.tsx b/packages/perseus/src/widgets/interactive-graphs/graphs/polygon.tsx index ba7635dbfc..92fda3124d 100644 --- a/packages/perseus/src/widgets/interactive-graphs/graphs/polygon.tsx +++ b/packages/perseus/src/widgets/interactive-graphs/graphs/polygon.tsx @@ -11,6 +11,7 @@ import {MovablePoint} from "./components/movable-point"; import {TextLabel} from "./components/text-label"; import {useDraggable} from "./use-draggable"; import {pixelsToVectors, useTransformVectorsToPixels} from "./use-transform"; +import {removeDuplicateCoordsFromArray} from "./utils"; import type {CollinearTuple} from "../../../perseus-types"; import type { @@ -254,11 +255,14 @@ const UnlimitedPolygonGraph = (props: Props) => { }} onClick={() => { // If the point being clicked is the first point and - // there's enough points to form a polygon (3 or more) - // Close the shape before setting focus. + // there's enough non-duplicated points to form + // a polygon (3 or more), close the shape before + // setting focus. if ( i === 0 && - props.graphState.coords.length >= 3 + removeDuplicateCoordsFromArray( + props.graphState.coords, + ).length >= 3 ) { dispatch(actions.polygon.closePolygon()); } diff --git a/packages/perseus/src/widgets/interactive-graphs/graphs/utils.test.ts b/packages/perseus/src/widgets/interactive-graphs/graphs/utils.test.ts index 87e7efcfa7..44ced7b5c2 100644 --- a/packages/perseus/src/widgets/interactive-graphs/graphs/utils.test.ts +++ b/packages/perseus/src/widgets/interactive-graphs/graphs/utils.test.ts @@ -1,5 +1,9 @@ -import {getIntersectionOfRayWithBox} from "./utils"; +import { + getIntersectionOfRayWithBox, + removeDuplicateCoordsFromArray, +} from "./utils"; +import type {Coord} from "@khanacademy/perseus"; import type {Interval, vec} from "mafs"; describe("getIntersectionOfRayWithBox", () => { @@ -139,3 +143,19 @@ describe("getIntersectionOfRayWithBox", () => { expect(intersection).toEqual([-1.11, -1.11]); }); }); + +describe("removeDuplicateCoordsFromArray", () => { + test("removes duplicate coordinates", () => { + const arr: Coord[] = [ + [0, 0], + [0, 0], + [1, 1], + ]; + + const result = removeDuplicateCoordsFromArray(arr); + expect(result).toEqual([ + [0, 0], + [1, 1], + ]); + }); +}); diff --git a/packages/perseus/src/widgets/interactive-graphs/graphs/utils.ts b/packages/perseus/src/widgets/interactive-graphs/graphs/utils.ts index 4c2fce42e4..613f0a9aa5 100644 --- a/packages/perseus/src/widgets/interactive-graphs/graphs/utils.ts +++ b/packages/perseus/src/widgets/interactive-graphs/graphs/utils.ts @@ -1,3 +1,4 @@ +import type {Coord} from "@khanacademy/perseus"; import type {Interval, vec} from "mafs"; /** @@ -44,3 +45,21 @@ export const getIntersectionOfRayWithBox = ( function isBetween(x: number, low: number, high: number) { return x >= low && x <= high; } + +export function removeDuplicateCoordsFromArray( + array: Array, +): Array { + const returnArray: Array = []; + + for (const coordPair of array) { + if ( + !returnArray.some( + ([x, y]) => x === coordPair[0] && y === coordPair[1], + ) + ) { + returnArray.push(coordPair); + } + } + + return returnArray; +} diff --git a/packages/perseus/src/widgets/interactive-graphs/mafs-graph.tsx b/packages/perseus/src/widgets/interactive-graphs/mafs-graph.tsx index 054e74bec8..518e6573e0 100644 --- a/packages/perseus/src/widgets/interactive-graphs/mafs-graph.tsx +++ b/packages/perseus/src/widgets/interactive-graphs/mafs-graph.tsx @@ -57,6 +57,7 @@ import type {vec} from "mafs"; import "mafs/core.css"; import "./mafs-styles.css"; +import {removeDuplicateCoordsFromArray} from "./graphs/utils"; export type MafsGraphProps = { flags?: APIOptions["flags"]; @@ -422,7 +423,8 @@ const renderPolygonGraphControls = (props: { // We want to disable the closePolygon button when // there are not enough coords to make a polygon - const disableCloseButton = coords.length < 3; + const disableCloseButton = + removeDuplicateCoordsFromArray(coords).length < 3; // If polygon is closed, show open button. // If polygon is open, show close button. 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 ca0e9220a3..9993464a20 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 @@ -15,6 +15,7 @@ import { vector, } from "../../../util/geometry"; import {getQuadraticCoefficients} from "../graphs/quadratic"; +import {removeDuplicateCoordsFromArray} from "../graphs/utils"; import { clamp, clampToBox, @@ -200,24 +201,11 @@ function doClickPoint( function doClosePolygon(state: InteractiveGraphState): InteractiveGraphState { if (isUnlimitedGraphState(state) && state.type === "polygon") { - // If the last point is the same as the first point when the - // "Close shape" button is clicked, we should remove the - // last point before closing the polygon. Otherwise, it would - // be counted as an extra vertex and the question would be - // marked wrong. (Making this consistent with the behavior if - // the user had clicked on the first point to close the shape.) - const lastPoint = state.coords[state.coords.length - 1]; - const firstPoint = state.coords[0]; - if (_.isEqual(lastPoint, firstPoint)) { - return { - ...state, - coords: state.coords.slice(0, -1), - closedPolygon: true, - }; - } + const nonDupePoints = removeDuplicateCoordsFromArray(state.coords); return { ...state, + coords: nonDupePoints, closedPolygon: true, }; } From 681c6674eb141b430450e3178925135a9cf5605f Mon Sep 17 00:00:00 2001 From: Nisha Yerunkar Date: Thu, 12 Dec 2024 13:12:30 -0800 Subject: [PATCH 4/8] Fix changeset and lint failures --- .changeset/small-mugs-bow.md | 6 ++++++ .../perseus-editor/src/__stories__/flags-for-api-options.ts | 2 -- .../perseus/src/widgets/interactive-graphs/mafs-graph.tsx | 2 +- 3 files changed, 7 insertions(+), 3 deletions(-) create mode 100644 .changeset/small-mugs-bow.md diff --git a/.changeset/small-mugs-bow.md b/.changeset/small-mugs-bow.md new file mode 100644 index 0000000000..ecbdbc7d25 --- /dev/null +++ b/.changeset/small-mugs-bow.md @@ -0,0 +1,6 @@ +--- +"@khanacademy/perseus": patch +"@khanacademy/perseus-editor": patch +--- + +[Polygon] Remove duplicate points when determining if a polygon can be closed diff --git a/packages/perseus-editor/src/__stories__/flags-for-api-options.ts b/packages/perseus-editor/src/__stories__/flags-for-api-options.ts index d3e0b62f9e..ab9acbb7fd 100644 --- a/packages/perseus-editor/src/__stories__/flags-for-api-options.ts +++ b/packages/perseus-editor/src/__stories__/flags-for-api-options.ts @@ -15,8 +15,6 @@ export const flags = { point: true, "unlimited-point": true, none: true, - "unlimited-point": true, - "unlimited-polygon": true, // Locked figures flags "interactive-graph-locked-features-labels": true, diff --git a/packages/perseus/src/widgets/interactive-graphs/mafs-graph.tsx b/packages/perseus/src/widgets/interactive-graphs/mafs-graph.tsx index 518e6573e0..e8fd618b4d 100644 --- a/packages/perseus/src/widgets/interactive-graphs/mafs-graph.tsx +++ b/packages/perseus/src/widgets/interactive-graphs/mafs-graph.tsx @@ -37,6 +37,7 @@ import {renderQuadraticGraph} from "./graphs/quadratic"; import {renderRayGraph} from "./graphs/ray"; import {renderSegmentGraph} from "./graphs/segment"; import {renderSinusoidGraph} from "./graphs/sinusoid"; +import {removeDuplicateCoordsFromArray} from "./graphs/utils"; import {MIN, X, Y} from "./math"; import {Protractor} from "./protractor"; import {actions} from "./reducer/interactive-graph-action"; @@ -57,7 +58,6 @@ import type {vec} from "mafs"; import "mafs/core.css"; import "./mafs-styles.css"; -import {removeDuplicateCoordsFromArray} from "./graphs/utils"; export type MafsGraphProps = { flags?: APIOptions["flags"]; From 8aaf423320fe75dda0b4d841eb76b5f12a3263ca Mon Sep 17 00:00:00 2001 From: Nisha Yerunkar Date: Thu, 12 Dec 2024 13:18:47 -0800 Subject: [PATCH 5/8] rename stuff --- .../src/widgets/interactive-graphs/graphs/polygon.tsx | 4 ++-- .../src/widgets/interactive-graphs/graphs/utils.test.ts | 4 ++-- .../perseus/src/widgets/interactive-graphs/graphs/utils.ts | 4 +--- .../perseus/src/widgets/interactive-graphs/mafs-graph.tsx | 4 ++-- .../interactive-graphs/reducer/interactive-graph-reducer.ts | 6 +++--- 5 files changed, 10 insertions(+), 12 deletions(-) diff --git a/packages/perseus/src/widgets/interactive-graphs/graphs/polygon.tsx b/packages/perseus/src/widgets/interactive-graphs/graphs/polygon.tsx index adcb6b2a96..b814a5e601 100644 --- a/packages/perseus/src/widgets/interactive-graphs/graphs/polygon.tsx +++ b/packages/perseus/src/widgets/interactive-graphs/graphs/polygon.tsx @@ -11,7 +11,7 @@ import {MovablePoint} from "./components/movable-point"; import {TextLabel} from "./components/text-label"; import {useDraggable} from "./use-draggable"; import {pixelsToVectors, useTransformVectorsToPixels} from "./use-transform"; -import {removeDuplicateCoordsFromArray} from "./utils"; +import {getArrayWithoutDuplicates} from "./utils"; import type {Coord} from "../../../interactive2/types"; import type {CollinearTuple} from "../../../perseus-types"; @@ -319,7 +319,7 @@ const UnlimitedPolygonGraph = (statefulProps: StatefulProps) => { // setting focus. if ( i === 0 && - removeDuplicateCoordsFromArray(coords).length >= 3 + getArrayWithoutDuplicates(coords).length >= 3 ) { dispatch(actions.polygon.closePolygon()); } diff --git a/packages/perseus/src/widgets/interactive-graphs/graphs/utils.test.ts b/packages/perseus/src/widgets/interactive-graphs/graphs/utils.test.ts index 44ced7b5c2..cc226462a1 100644 --- a/packages/perseus/src/widgets/interactive-graphs/graphs/utils.test.ts +++ b/packages/perseus/src/widgets/interactive-graphs/graphs/utils.test.ts @@ -1,6 +1,6 @@ import { getIntersectionOfRayWithBox, - removeDuplicateCoordsFromArray, + getArrayWithoutDuplicates, } from "./utils"; import type {Coord} from "@khanacademy/perseus"; @@ -152,7 +152,7 @@ describe("removeDuplicateCoordsFromArray", () => { [1, 1], ]; - const result = removeDuplicateCoordsFromArray(arr); + const result = getArrayWithoutDuplicates(arr); expect(result).toEqual([ [0, 0], [1, 1], diff --git a/packages/perseus/src/widgets/interactive-graphs/graphs/utils.ts b/packages/perseus/src/widgets/interactive-graphs/graphs/utils.ts index 613f0a9aa5..c195f00759 100644 --- a/packages/perseus/src/widgets/interactive-graphs/graphs/utils.ts +++ b/packages/perseus/src/widgets/interactive-graphs/graphs/utils.ts @@ -46,9 +46,7 @@ function isBetween(x: number, low: number, high: number) { return x >= low && x <= high; } -export function removeDuplicateCoordsFromArray( - array: Array, -): Array { +export function getArrayWithoutDuplicates(array: Array): Array { const returnArray: Array = []; for (const coordPair of array) { diff --git a/packages/perseus/src/widgets/interactive-graphs/mafs-graph.tsx b/packages/perseus/src/widgets/interactive-graphs/mafs-graph.tsx index e8fd618b4d..34d5e464ca 100644 --- a/packages/perseus/src/widgets/interactive-graphs/mafs-graph.tsx +++ b/packages/perseus/src/widgets/interactive-graphs/mafs-graph.tsx @@ -37,7 +37,7 @@ import {renderQuadraticGraph} from "./graphs/quadratic"; import {renderRayGraph} from "./graphs/ray"; import {renderSegmentGraph} from "./graphs/segment"; import {renderSinusoidGraph} from "./graphs/sinusoid"; -import {removeDuplicateCoordsFromArray} from "./graphs/utils"; +import {getArrayWithoutDuplicates} from "./graphs/utils"; import {MIN, X, Y} from "./math"; import {Protractor} from "./protractor"; import {actions} from "./reducer/interactive-graph-action"; @@ -424,7 +424,7 @@ const renderPolygonGraphControls = (props: { // We want to disable the closePolygon button when // there are not enough coords to make a polygon const disableCloseButton = - removeDuplicateCoordsFromArray(coords).length < 3; + getArrayWithoutDuplicates(coords).length < 3; // If polygon is closed, show open button. // If polygon is open, show close button. 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 9993464a20..a1bc2042fa 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 @@ -15,7 +15,7 @@ import { vector, } from "../../../util/geometry"; import {getQuadraticCoefficients} from "../graphs/quadratic"; -import {removeDuplicateCoordsFromArray} from "../graphs/utils"; +import {getArrayWithoutDuplicates} from "../graphs/utils"; import { clamp, clampToBox, @@ -201,11 +201,11 @@ function doClickPoint( function doClosePolygon(state: InteractiveGraphState): InteractiveGraphState { if (isUnlimitedGraphState(state) && state.type === "polygon") { - const nonDupePoints = removeDuplicateCoordsFromArray(state.coords); + const noDupedPoints = getArrayWithoutDuplicates(state.coords); return { ...state, - coords: nonDupePoints, + coords: noDupedPoints, closedPolygon: true, }; } From 04a82645f01bf69768a90adfbbf0abee67ffacb1 Mon Sep 17 00:00:00 2001 From: Nisha Yerunkar Date: Thu, 12 Dec 2024 13:49:10 -0800 Subject: [PATCH 6/8] Add tests --- .../interactive-graphs/graphs/utils.test.ts | 49 ++++++++- .../interactive-graphs/graphs/utils.ts | 1 + .../interactive-graphs/mafs-graph.test.tsx | 101 ++++++++++++++++++ .../reducer/interactive-graph-reducer.test.ts | 28 ++++- .../reducer/interactive-graph-reducer.ts | 4 + 5 files changed, 178 insertions(+), 5 deletions(-) diff --git a/packages/perseus/src/widgets/interactive-graphs/graphs/utils.test.ts b/packages/perseus/src/widgets/interactive-graphs/graphs/utils.test.ts index cc226462a1..eaa274fd14 100644 --- a/packages/perseus/src/widgets/interactive-graphs/graphs/utils.test.ts +++ b/packages/perseus/src/widgets/interactive-graphs/graphs/utils.test.ts @@ -1,7 +1,4 @@ -import { - getIntersectionOfRayWithBox, - getArrayWithoutDuplicates, -} from "./utils"; +import {getIntersectionOfRayWithBox, getArrayWithoutDuplicates} from "./utils"; import type {Coord} from "@khanacademy/perseus"; import type {Interval, vec} from "mafs"; @@ -146,16 +143,60 @@ describe("getIntersectionOfRayWithBox", () => { describe("removeDuplicateCoordsFromArray", () => { test("removes duplicate coordinates", () => { + // Arrange const arr: Coord[] = [ [0, 0], [0, 0], [1, 1], ]; + // Act const result = getArrayWithoutDuplicates(arr); + + // Assert expect(result).toEqual([ [0, 0], [1, 1], ]); }); + + test("removes many duplicate coordinates", () => { + // Arrange + const arr: Coord[] = [ + [0, 0], + [1, 1], + [0, 0], + [1, 1], + [0, 0], + [1, 1], + ]; + + // Act + const result = getArrayWithoutDuplicates(arr); + + // Assert + expect(result).toEqual([ + [0, 0], + [1, 1], + ]); + }); + + test("does not remove unique coordinates", () => { + // Arrange + const arr: Coord[] = [ + [0, 1], + [1, 2], + [2, 3], + ]; + + // Act + const result = getArrayWithoutDuplicates(arr); + + // Assert + expect(result).toEqual([ + [0, 1], + [1, 2], + [2, 3], + ]); + }); }); diff --git a/packages/perseus/src/widgets/interactive-graphs/graphs/utils.ts b/packages/perseus/src/widgets/interactive-graphs/graphs/utils.ts index c195f00759..d42852ec91 100644 --- a/packages/perseus/src/widgets/interactive-graphs/graphs/utils.ts +++ b/packages/perseus/src/widgets/interactive-graphs/graphs/utils.ts @@ -51,6 +51,7 @@ export function getArrayWithoutDuplicates(array: Array): Array { for (const coordPair of array) { if ( + // Check if the coordPair is not already in the returnArray !returnArray.some( ([x, y]) => x === coordPair[0] && y === coordPair[1], ) 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 2d5069e3c7..15e059a716 100644 --- a/packages/perseus/src/widgets/interactive-graphs/mafs-graph.test.tsx +++ b/packages/perseus/src/widgets/interactive-graphs/mafs-graph.test.tsx @@ -947,6 +947,107 @@ describe("MafsGraph", () => { {type: REMOVE_POINT, index: 0}, ]); }); + + it("polygon - enables the 'Close shape' button when the polygon has 3 or more unique points", () => { + // Arrange + // Render the question + const mockDispatch = jest.fn(); + const state: InteractiveGraphState = { + type: "polygon", + numSides: "unlimited", + focusedPointIndex: null, + hasBeenInteractedWith: true, + showRemovePointButton: false, + interactionMode: "mouse", + showKeyboardInteractionInvitation: false, + showAngles: false, + showSides: false, + range: [ + [-10, 10], + [-10, 10], + ], + snapStep: [2, 2], + snapTo: "grid", + coords: [ + [4, 5], + [5, 6], + [6, 7], + ], + closedPolygon: false, + }; + + const baseMafsGraphProps: MafsGraphProps = { + ...getBaseMafsGraphProps(), + markings: "none", + }; + + render( + , + ); + + // Assert + // Find the button + const closeShapeButton = screen.getByRole("button", { + name: "Close shape", + }); + // Make sure the button is enabled + expect(closeShapeButton).toHaveAttribute("aria-disabled", "false"); + }); + + it("polygon - disables the 'Close shape' button when the polygon has fewer than 3 unique points", () => { + // Arrange + // Render the question + const mockDispatch = jest.fn(); + const state: InteractiveGraphState = { + type: "polygon", + numSides: "unlimited", + focusedPointIndex: null, + hasBeenInteractedWith: true, + showRemovePointButton: false, + interactionMode: "mouse", + showKeyboardInteractionInvitation: false, + showAngles: false, + showSides: false, + range: [ + [-10, 10], + [-10, 10], + ], + snapStep: [2, 2], + snapTo: "grid", + coords: [ + [4, 5], + [5, 6], + // not unique + [5, 6], + ], + closedPolygon: false, + }; + + const baseMafsGraphProps: MafsGraphProps = { + ...getBaseMafsGraphProps(), + markings: "none", + }; + + render( + , + ); + + // Assert + // Find the button + const closeShapeButton = screen.getByRole("button", { + name: "Close shape", + }); + // Make sure the button is disabled + expect(closeShapeButton).toHaveAttribute("aria-disabled", "true"); + }); }); }); 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 index 2b5a03dce7..d16f6c17a4 100644 --- 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 @@ -1548,7 +1548,7 @@ describe("doClosePolygon", () => { expect(updated.closedPolygon).toBeTruthy(); }); - it("does not change `closedPolygon` property when it's already false", () => { + it("does not change `closedPolygon` property when it's already true", () => { const state: InteractiveGraphState = { ...baseUnlimitedPolygonGraphState, closedPolygon: true, @@ -1562,6 +1562,32 @@ describe("doClosePolygon", () => { invariant(updated.type === "polygon"); expect(updated.closedPolygon).toBeTruthy(); }); + + it("removes duplicated points from the new state when closed", () => { + const state: InteractiveGraphState = { + ...baseUnlimitedPolygonGraphState, + coords: [ + [0, 0], + [0, 1], + [1, 1], + [0, 0], // last point same as first point + ], + closedPolygon: false, + }; + + const updated = interactiveGraphReducer( + state, + actions.polygon.closePolygon(), + ); + + invariant(updated.type === "polygon"); + expect(updated.closedPolygon).toBeTruthy(); + expect(updated.coords).toEqual([ + [0, 0], + [0, 1], + [1, 1], + ]); + }); }); describe("doOpenPolygon", () => { 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 a1bc2042fa..5b5e6974c7 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 @@ -201,6 +201,10 @@ function doClickPoint( function doClosePolygon(state: InteractiveGraphState): InteractiveGraphState { if (isUnlimitedGraphState(state) && state.type === "polygon") { + // We want to remove any duplicate points when closing the polygon to + // (1) prevent the polygon from sides with length zero, and + // (2) make sure the question is can be marked correct if the polygon + // LOOKS correct, even if two of the points are at the same coords. const noDupedPoints = getArrayWithoutDuplicates(state.coords); return { From f3f466388d5646737eb0aed4eb6dd9c453af9e49 Mon Sep 17 00:00:00 2001 From: Nisha Yerunkar Date: Thu, 12 Dec 2024 14:11:12 -0800 Subject: [PATCH 7/8] prettier --- packages/perseus/src/widgets/interactive-graphs/mafs-graph.tsx | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/perseus/src/widgets/interactive-graphs/mafs-graph.tsx b/packages/perseus/src/widgets/interactive-graphs/mafs-graph.tsx index 34d5e464ca..42fa179f5e 100644 --- a/packages/perseus/src/widgets/interactive-graphs/mafs-graph.tsx +++ b/packages/perseus/src/widgets/interactive-graphs/mafs-graph.tsx @@ -423,8 +423,7 @@ const renderPolygonGraphControls = (props: { // We want to disable the closePolygon button when // there are not enough coords to make a polygon - const disableCloseButton = - getArrayWithoutDuplicates(coords).length < 3; + const disableCloseButton = getArrayWithoutDuplicates(coords).length < 3; // If polygon is closed, show open button. // If polygon is open, show close button. From 0cc8c19c48e844cc35b43af2a056b3d141e174fb Mon Sep 17 00:00:00 2001 From: Nisha Yerunkar Date: Mon, 16 Dec 2024 11:02:45 -0800 Subject: [PATCH 8/8] fix from merge --- .../src/widgets/interactive-graphs/mafs-graph.test.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 59382295e5..941711c4c6 100644 --- a/packages/perseus/src/widgets/interactive-graphs/mafs-graph.test.tsx +++ b/packages/perseus/src/widgets/interactive-graphs/mafs-graph.test.tsx @@ -952,7 +952,7 @@ describe("MafsGraph", () => { }; const baseMafsGraphProps: MafsGraphProps = { - ...getBaseMafsGraphProps(), + ...getBaseMafsGraphPropsForTests(), markings: "none", }; @@ -1003,7 +1003,7 @@ describe("MafsGraph", () => { }; const baseMafsGraphProps: MafsGraphProps = { - ...getBaseMafsGraphProps(), + ...getBaseMafsGraphPropsForTests(), markings: "none", };