Skip to content

Commit 81632c3

Browse files
nishasynishasy
authored
[Polygon] Remove duplicate points when determining if a polygon can be closed (#1978)
## Summary: Right now, if a user were trying to close the polygon by clicking the last point, missed, created a new point instead as a result, and then tried to drag the new point to the first point to close out the polygon, the question would be marked wrong even if the polygon looks right. Relatedly, if a user were to drag a middle point over another point in the polygon, even if the polygon looks right, it would be marked wrong. To fix both of these issues, I'm making it so - the close button is only enabled if there are three or more _unique_ points in the polygon - closing the polygon removes any duplicate points from the coords array. This allows the dragging to work more intuitively and the question to be graded fairly (based on how the polygon looks regardless of duplicate points) Issue: https://khanacademy.atlassian.net/browse/LEMS-2711 (and also https://khanacademy.atlassian.net/browse/LEMS-2722) ## Test plan: `yarn jest packages/perseus/src/widgets/interactive-graphs/graphs/utils.test.ts` `yarn jest packages/perseus/src/widgets/interactive-graphs/mafs-graph.test.tsx` `yarn jest packages/perseus/src/widgets/interactive-graphs/reducer/interactive-graph-reducer.test.ts` Storybook - Go to http://localhost:6006/?path=/story/perseus-widgets-interactive-graph--unlimited-polygon-with-mafs - Put down four points - Click to create a fifth point - drag the fifth point over to the first one - close the shape with the button - re-open the shape - confirm that the previous fifth point is now gone - move a middle point over another middle point - close and reopen the shape - confirm that the duplicate point is gone - get to a state of three points - move one point over another so it looks like there are two points - confirm that the close button is disabled ### Before: https://github.com/user-attachments/assets/2e26ca48-5925-4738-96a2-91adf71b2826 ### After: https://github.com/user-attachments/assets/66dc76f1-4207-47e1-9ab8-2ef5a8f781a6 Author: nishasy Reviewers: catandthemachines, nishasy, anakaren-rojas Required Reviewers: Approved By: catandthemachines Checks: ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x) Pull Request URL: #1978
1 parent 763d2d0 commit 81632c3

File tree

10 files changed

+235
-9
lines changed

10 files changed

+235
-9
lines changed

‎.changeset/small-mugs-bow.md

+6
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
---
2+
"@khanacademy/perseus": patch
3+
"@khanacademy/perseus-editor": patch
4+
---
5+
6+
[Polygon] Remove duplicate points when determining if a polygon can be closed

‎packages/perseus-editor/src/__stories__/interactive-graph-editor.stories.tsx

+2-2
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ import {
2222
segmentWithStartingCoordsQuestion,
2323
segmentsWithStartingCoordsQuestion,
2424
sinusoidWithStartingCoordsQuestion,
25-
unlimitedPolygonWithStartingCoordsQuestion,
25+
unlimitedPolygonWithCorrectAnswerQuestion,
2626
} from "../../../perseus/src/widgets/interactive-graphs/interactive-graph.testdata";
2727
import {registerAllWidgetsAndEditorsForTesting} from "../util/register-all-widgets-and-editors-for-testing";
2828

@@ -129,7 +129,7 @@ export const InteractiveGraphPolygon = (): React.ReactElement => {
129129
export const InteractiveGraphUnlimitedPolygon = (): React.ReactElement => {
130130
return (
131131
<EditorPageWithStorybookPreview
132-
question={unlimitedPolygonWithStartingCoordsQuestion}
132+
question={unlimitedPolygonWithCorrectAnswerQuestion}
133133
/>
134134
);
135135
};

‎packages/perseus/src/widgets/interactive-graphs/graphs/polygon.tsx

+8-3
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import {MovablePoint} from "./components/movable-point";
1111
import {TextLabel} from "./components/text-label";
1212
import {useDraggable} from "./use-draggable";
1313
import {pixelsToVectors, useTransformVectorsToPixels} from "./use-transform";
14+
import {getArrayWithoutDuplicates} from "./utils";
1415

1516
import type {Coord} from "../../../interactive2/types";
1617
import type {CollinearTuple} from "../../../perseus-types";
@@ -322,9 +323,13 @@ const UnlimitedPolygonGraph = (statefulProps: StatefulProps) => {
322323
}}
323324
onClick={() => {
324325
// If the point being clicked is the first point and
325-
// there's enough points to form a polygon (3 or more)
326-
// Close the shape before setting focus.
327-
if (i === 0 && coords.length >= 3) {
326+
// there's enough non-duplicated points to form
327+
// a polygon (3 or more), close the shape before
328+
// setting focus.
329+
if (
330+
i === 0 &&
331+
getArrayWithoutDuplicates(coords).length >= 3
332+
) {
328333
dispatch(actions.polygon.closePolygon());
329334
}
330335
dispatch(actions.polygon.clickPoint(i));

‎packages/perseus/src/widgets/interactive-graphs/graphs/utils.test.ts

+62-1
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
1-
import {getIntersectionOfRayWithBox} from "./utils";
1+
import {getIntersectionOfRayWithBox, getArrayWithoutDuplicates} from "./utils";
22

3+
import type {Coord} from "@khanacademy/perseus";
34
import type {Interval, vec} from "mafs";
45

56
describe("getIntersectionOfRayWithBox", () => {
@@ -139,3 +140,63 @@ describe("getIntersectionOfRayWithBox", () => {
139140
expect(intersection).toEqual([-1.11, -1.11]);
140141
});
141142
});
143+
144+
describe("removeDuplicateCoordsFromArray", () => {
145+
test("removes duplicate coordinates", () => {
146+
// Arrange
147+
const arr: Coord[] = [
148+
[0, 0],
149+
[0, 0],
150+
[1, 1],
151+
];
152+
153+
// Act
154+
const result = getArrayWithoutDuplicates(arr);
155+
156+
// Assert
157+
expect(result).toEqual([
158+
[0, 0],
159+
[1, 1],
160+
]);
161+
});
162+
163+
test("removes many duplicate coordinates", () => {
164+
// Arrange
165+
const arr: Coord[] = [
166+
[0, 0],
167+
[1, 1],
168+
[0, 0],
169+
[1, 1],
170+
[0, 0],
171+
[1, 1],
172+
];
173+
174+
// Act
175+
const result = getArrayWithoutDuplicates(arr);
176+
177+
// Assert
178+
expect(result).toEqual([
179+
[0, 0],
180+
[1, 1],
181+
]);
182+
});
183+
184+
test("does not remove unique coordinates", () => {
185+
// Arrange
186+
const arr: Coord[] = [
187+
[0, 1],
188+
[1, 2],
189+
[2, 3],
190+
];
191+
192+
// Act
193+
const result = getArrayWithoutDuplicates(arr);
194+
195+
// Assert
196+
expect(result).toEqual([
197+
[0, 1],
198+
[1, 2],
199+
[2, 3],
200+
]);
201+
});
202+
});

‎packages/perseus/src/widgets/interactive-graphs/graphs/utils.ts

+18
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import type {Coord} from "@khanacademy/perseus";
12
import type {Interval, vec} from "mafs";
23

34
/**
@@ -44,3 +45,20 @@ export const getIntersectionOfRayWithBox = (
4445
function isBetween(x: number, low: number, high: number) {
4546
return x >= low && x <= high;
4647
}
48+
49+
export function getArrayWithoutDuplicates(array: Array<Coord>): Array<Coord> {
50+
const returnArray: Array<Coord> = [];
51+
52+
for (const coordPair of array) {
53+
if (
54+
// Check if the coordPair is not already in the returnArray
55+
!returnArray.some(
56+
([x, y]) => x === coordPair[0] && y === coordPair[1],
57+
)
58+
) {
59+
returnArray.push(coordPair);
60+
}
61+
}
62+
63+
return returnArray;
64+
}

‎packages/perseus/src/widgets/interactive-graphs/interactive-graph.testdata.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -193,7 +193,7 @@ export const polygonWithStartingCoordsQuestion: PerseusRenderer =
193193
})
194194
.build();
195195

196-
export const unlimitedPolygonWithStartingCoordsQuestion: PerseusRenderer =
196+
export const unlimitedPolygonWithCorrectAnswerQuestion: PerseusRenderer =
197197
interactiveGraphQuestionBuilder()
198198
.withPolygon("grid", {
199199
numSides: "unlimited",

‎packages/perseus/src/widgets/interactive-graphs/mafs-graph.test.tsx

+101
Original file line numberDiff line numberDiff line change
@@ -922,6 +922,107 @@ describe("MafsGraph", () => {
922922
{type: REMOVE_POINT, index: 0},
923923
]);
924924
});
925+
926+
it("polygon - enables the 'Close shape' button when the polygon has 3 or more unique points", () => {
927+
// Arrange
928+
// Render the question
929+
const mockDispatch = jest.fn();
930+
const state: InteractiveGraphState = {
931+
type: "polygon",
932+
numSides: "unlimited",
933+
focusedPointIndex: null,
934+
hasBeenInteractedWith: true,
935+
showRemovePointButton: false,
936+
interactionMode: "mouse",
937+
showKeyboardInteractionInvitation: false,
938+
showAngles: false,
939+
showSides: false,
940+
range: [
941+
[-10, 10],
942+
[-10, 10],
943+
],
944+
snapStep: [2, 2],
945+
snapTo: "grid",
946+
coords: [
947+
[4, 5],
948+
[5, 6],
949+
[6, 7],
950+
],
951+
closedPolygon: false,
952+
};
953+
954+
const baseMafsGraphProps: MafsGraphProps = {
955+
...getBaseMafsGraphPropsForTests(),
956+
markings: "none",
957+
};
958+
959+
render(
960+
<MafsGraph
961+
{...baseMafsGraphProps}
962+
state={state}
963+
dispatch={mockDispatch}
964+
/>,
965+
);
966+
967+
// Assert
968+
// Find the button
969+
const closeShapeButton = screen.getByRole("button", {
970+
name: "Close shape",
971+
});
972+
// Make sure the button is enabled
973+
expect(closeShapeButton).toHaveAttribute("aria-disabled", "false");
974+
});
975+
976+
it("polygon - disables the 'Close shape' button when the polygon has fewer than 3 unique points", () => {
977+
// Arrange
978+
// Render the question
979+
const mockDispatch = jest.fn();
980+
const state: InteractiveGraphState = {
981+
type: "polygon",
982+
numSides: "unlimited",
983+
focusedPointIndex: null,
984+
hasBeenInteractedWith: true,
985+
showRemovePointButton: false,
986+
interactionMode: "mouse",
987+
showKeyboardInteractionInvitation: false,
988+
showAngles: false,
989+
showSides: false,
990+
range: [
991+
[-10, 10],
992+
[-10, 10],
993+
],
994+
snapStep: [2, 2],
995+
snapTo: "grid",
996+
coords: [
997+
[4, 5],
998+
[5, 6],
999+
// not unique
1000+
[5, 6],
1001+
],
1002+
closedPolygon: false,
1003+
};
1004+
1005+
const baseMafsGraphProps: MafsGraphProps = {
1006+
...getBaseMafsGraphPropsForTests(),
1007+
markings: "none",
1008+
};
1009+
1010+
render(
1011+
<MafsGraph
1012+
{...baseMafsGraphProps}
1013+
state={state}
1014+
dispatch={mockDispatch}
1015+
/>,
1016+
);
1017+
1018+
// Assert
1019+
// Find the button
1020+
const closeShapeButton = screen.getByRole("button", {
1021+
name: "Close shape",
1022+
});
1023+
// Make sure the button is disabled
1024+
expect(closeShapeButton).toHaveAttribute("aria-disabled", "true");
1025+
});
9251026
});
9261027
});
9271028

‎packages/perseus/src/widgets/interactive-graphs/mafs-graph.tsx

+2-1
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ import {renderQuadraticGraph} from "./graphs/quadratic";
3737
import {renderRayGraph} from "./graphs/ray";
3838
import {renderSegmentGraph} from "./graphs/segment";
3939
import {renderSinusoidGraph} from "./graphs/sinusoid";
40+
import {getArrayWithoutDuplicates} from "./graphs/utils";
4041
import {MIN, X, Y} from "./math";
4142
import {Protractor} from "./protractor";
4243
import {actions} from "./reducer/interactive-graph-action";
@@ -422,7 +423,7 @@ const renderPolygonGraphControls = (props: {
422423

423424
// We want to disable the closePolygon button when
424425
// there are not enough coords to make a polygon
425-
const disableCloseButton = coords.length < 3;
426+
const disableCloseButton = getArrayWithoutDuplicates(coords).length < 3;
426427

427428
// If polygon is closed, show open button.
428429
// If polygon is open, show close button.

‎packages/perseus/src/widgets/interactive-graphs/reducer/interactive-graph-reducer.test.ts

+27-1
Original file line numberDiff line numberDiff line change
@@ -1548,7 +1548,7 @@ describe("doClosePolygon", () => {
15481548
expect(updated.closedPolygon).toBeTruthy();
15491549
});
15501550

1551-
it("does not change `closedPolygon` property when it's already false", () => {
1551+
it("does not change `closedPolygon` property when it's already true", () => {
15521552
const state: InteractiveGraphState = {
15531553
...baseUnlimitedPolygonGraphState,
15541554
closedPolygon: true,
@@ -1562,6 +1562,32 @@ describe("doClosePolygon", () => {
15621562
invariant(updated.type === "polygon");
15631563
expect(updated.closedPolygon).toBeTruthy();
15641564
});
1565+
1566+
it("removes duplicated points from the new state when closed", () => {
1567+
const state: InteractiveGraphState = {
1568+
...baseUnlimitedPolygonGraphState,
1569+
coords: [
1570+
[0, 0],
1571+
[0, 1],
1572+
[1, 1],
1573+
[0, 0], // last point same as first point
1574+
],
1575+
closedPolygon: false,
1576+
};
1577+
1578+
const updated = interactiveGraphReducer(
1579+
state,
1580+
actions.polygon.closePolygon(),
1581+
);
1582+
1583+
invariant(updated.type === "polygon");
1584+
expect(updated.closedPolygon).toBeTruthy();
1585+
expect(updated.coords).toEqual([
1586+
[0, 0],
1587+
[0, 1],
1588+
[1, 1],
1589+
]);
1590+
});
15651591
});
15661592

15671593
describe("doOpenPolygon", () => {

‎packages/perseus/src/widgets/interactive-graphs/reducer/interactive-graph-reducer.ts

+8
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import {
1515
vector,
1616
} from "../../../util/geometry";
1717
import {getQuadraticCoefficients} from "../graphs/quadratic";
18+
import {getArrayWithoutDuplicates} from "../graphs/utils";
1819
import {
1920
clamp,
2021
clampToBox,
@@ -200,8 +201,15 @@ function doClickPoint(
200201

201202
function doClosePolygon(state: InteractiveGraphState): InteractiveGraphState {
202203
if (isUnlimitedGraphState(state) && state.type === "polygon") {
204+
// We want to remove any duplicate points when closing the polygon to
205+
// (1) prevent the polygon from sides with length zero, and
206+
// (2) make sure the question is can be marked correct if the polygon
207+
// LOOKS correct, even if two of the points are at the same coords.
208+
const noDupedPoints = getArrayWithoutDuplicates(state.coords);
209+
203210
return {
204211
...state,
212+
coords: noDupedPoints,
205213
closedPolygon: true,
206214
};
207215
}

0 commit comments

Comments
 (0)