Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Hint Mode: Start Coords] Add start coords UI for polygon graphs (snap to grid only) #1488

Merged
merged 2 commits into from
Aug 6, 2024
Merged

[Hint Mode: Start Coords] Add start coords UI for polygon graphs (snap to grid only) #1488

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5429fa0
[Hint Mode: Start Coords] Add start coords UI for polygon graphs (sna…
nishasy Aug 5, 2024
18e9c85
use correct test name
nishasy Aug 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changeset/unlucky-lamps-draw.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"@khanacademy/perseus": minor
"@khanacademy/perseus-editor": minor
---

[Hint Mode: Start Coords] Add start coords UI for polygon graphs (snap to grid only)
1 change: 1 addition & 0 deletions packages/perseus-editor/src/__stories__/flags-for-api-options.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ export const flags = {
"start-coords-ui-phase-1": true,
"start-coords-ui-phase-2": true,
"start-coords-ui-point": true,
"start-coords-ui-polygon": true,
},
} satisfies APIOptions["flags"];

Expand Down
86 changes: 86 additions & 0 deletions packages/perseus-editor/src/components/__tests__/start-coords-settings.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -606,4 +606,90 @@ describe("StartCoordSettings", () => {
},
);
});

describe("polygon graph", () => {
test("shows the start coordinates UI: 3 sides (default)", () => {
// Arrange

// Act
render(
<StartCoordsSettings
{...defaultProps}
type="polygon"
onChange={() => {}}
/>,
{wrapper: RenderStateRoot},
);

// Assert
expect(screen.getByText("Start coordinates")).toBeInTheDocument();
expect(screen.getByText("Point 1:")).toBeInTheDocument();
expect(screen.getByText("Point 2:")).toBeInTheDocument();
expect(screen.getByText("Point 3:")).toBeInTheDocument();
});

test("shows the start coordinates UI: 6 sides", () => {
// Arrange

// Act
render(
<StartCoordsSettings
{...defaultProps}
type="polygon"
numSides={6}
onChange={() => {}}
/>,
{wrapper: RenderStateRoot},
);

// Assert
expect(screen.getByText("Start coordinates")).toBeInTheDocument();
expect(screen.getByText("Point 1:")).toBeInTheDocument();
expect(screen.getByText("Point 2:")).toBeInTheDocument();
expect(screen.getByText("Point 3:")).toBeInTheDocument();
expect(screen.getByText("Point 4:")).toBeInTheDocument();
expect(screen.getByText("Point 5:")).toBeInTheDocument();
expect(screen.getByText("Point 6:")).toBeInTheDocument();
});

test.each`
pointIndex | coord
${0} | ${"x"}
${0} | ${"y"}
${1} | ${"x"}
${1} | ${"y"}
${2} | ${"x"}
${2} | ${"y"}
`(
"calls onChange when $coord coord is changed (line $pointIndex)",
async ({pointIndex, coord}) => {
// Arrange
const onChangeMock = jest.fn();
render(
<StartCoordsSettings
{...defaultProps}
type="polygon"
onChange={onChangeMock}
/>,
);

// Act
const input = screen.getAllByRole("spinbutton", {
name: `${coord}`,
})[pointIndex];
await userEvent.clear(input);
await userEvent.type(input, "101");

// Assert
const expectedCoords = [
[3, -2],
[0, 4],
[-3, -2],
];
expectedCoords[pointIndex][coord === "x" ? 0 : 1] = 101;

expect(onChangeMock).toHaveBeenLastCalledWith(expectedCoords);
},
);
});
});
39 changes: 39 additions & 0 deletions packages/perseus-editor/src/components/__tests__/util.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -326,6 +326,45 @@ describe("getDefaultGraphStartCoords", () => {
[5, 0],
]);
});

test("should get default start coords for a polygon graph, triangle (default)", () => {
// Arrange
const graph: PerseusGraphType = {type: "polygon"};
const range = [
[-10, 10],
[-10, 10],
] satisfies [Range, Range];
const step = [1, 1] satisfies [number, number];

// Act
const defaultCoords = getDefaultGraphStartCoords(graph, range, step);

expect(defaultCoords).toEqual([
[3, -2],
[0, 4],
[-3, -2],
]);
});

test("should get default start coords for a polygon graph, square", () => {
// Arrange
const graph: PerseusGraphType = {type: "polygon", numSides: 4};
const range = [
[-10, 10],
[-10, 10],
] satisfies [Range, Range];
const step = [1, 1] satisfies [number, number];

// Act
const defaultCoords = getDefaultGraphStartCoords(graph, range, step);

expect(defaultCoords).toEqual([
[3, -3],
[3, 3],
[-3, 3],
[-3, -3],
]);
});
});

describe("getSinusoidEquation", () => {
Expand Down
8 changes: 7 additions & 1 deletion packages/perseus-editor/src/components/start-coords-settings.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import {
getLineCoords,
getLinearSystemCoords,
getPointCoords,
getPolygonCoords,
getQuadraticCoords,
getSegmentCoords,
getSinusoidCoords,
Expand Down Expand Up @@ -91,8 +92,13 @@ const StartCoordsSettingsInner = (props: Props) => {
onChange={onChange}
/>
);
// Graphs with startCoords of type ReadonlyArray<Coord>
case "point":
const pointCoords = getPointCoords(props, range, step);
case "polygon":
const pointCoords =
type === "point"
? getPointCoords(props, range, step)
: getPolygonCoords(props, range, step);
return (
<StartCoordsPoint
startCoords={pointCoords}
Expand Down
19 changes: 19 additions & 0 deletions packages/perseus-editor/src/components/util.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import {
getLineCoords,
getLinearSystemCoords,
getPointCoords,
getPolygonCoords,
getQuadraticCoords,
getSegmentCoords,
getSinusoidCoords,
Expand Down Expand Up @@ -201,6 +202,12 @@ export function getDefaultGraphStartCoords(
range,
step,
);
case "polygon":
return getPolygonCoords(
{...graph, startCoords: undefined},
range,
step,
);
default:
return undefined;
}
Expand Down Expand Up @@ -275,6 +282,7 @@ export const shouldShowStartCoordsUI = (flags, graph) => {
const startCoordsPhase1 = flags?.mafs?.["start-coords-ui-phase-1"];
const startCoordsPhase2 = flags?.mafs?.["start-coords-ui-phase-2"];
const startCoordsPoint = flags?.mafs?.["start-coords-ui-point"];
const startCoordsPolygon = flags?.mafs?.["start-coords-ui-polygon"];

if (startCoordsPhase1 && startCoordsUiPhase1Types.includes(graph.type)) {
return true;
Expand All @@ -292,5 +300,16 @@ export const shouldShowStartCoordsUI = (flags, graph) => {
return true;
}

if (
startCoordsPolygon &&
graph.type === "polygon" &&
graph.numPoints !== "unlimited" &&
// Pre-initialized graph with undefined snapTo value
// initializes to snapTo="grid"
(graph.snapTo === "grid" || graph.snapTo === undefined)
) {
return true;
}

return false;
};
70 changes: 66 additions & 4 deletions packages/perseus-editor/src/widgets/__tests__/interactive-graph-editor.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -700,9 +700,10 @@ describe("InteractiveGraphEditor", () => {
...flags,
mafs: {
...flags.mafs,
"start-coords-ui-phase-1": shouldRender,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm so embarrassed by how broken these tests were. They basically weren't even testing anything! 😬

"start-coords-ui-phase-1": true,
"start-coords-ui-phase-2": false,
"start-coords-ui-point": false,
"start-coords-ui-polygon": false,
},
},
}}
Expand Down Expand Up @@ -738,7 +739,7 @@ describe("InteractiveGraphEditor", () => {
${"linear-system"} | ${false}
${"segment"} | ${false}
${"circle"} | ${false}
${"quadratic"} | ${false}
${"quadratic"} | ${true}
${"sinusoid"} | ${true}
${"polygon"} | ${false}
${"angle"} | ${false}
Expand All @@ -759,8 +760,9 @@ describe("InteractiveGraphEditor", () => {
mafs: {
...flags.mafs,
"start-coords-ui-phase-1": false,
"start-coords-ui-phase-2": shouldRender,
"start-coords-ui-phase-2": true,
"start-coords-ui-point": false,
"start-coords-ui-polygon": false,
},
},
}}
Expand Down Expand Up @@ -818,7 +820,67 @@ describe("InteractiveGraphEditor", () => {
...flags.mafs,
"start-coords-ui-phase-1": false,
"start-coords-ui-phase-2": false,
"start-coords-ui-point": shouldRender,
"start-coords-ui-point": true,
"start-coords-ui-polygon": false,
},
},
}}
graph={{type}}
correct={{type}}
/>,
{
wrapper: RenderStateRoot,
},
);

// Assert
if (shouldRender) {
expect(
await screen.findByRole("button", {
name: "Use default start coordinates",
}),
).toBeInTheDocument();
} else {
expect(
screen.queryByRole("button", {
name: "Use default start coordinates",
}),
).toBeNull();
}
},
);

test.each`
type | shouldRender
${"linear"} | ${false}
${"ray"} | ${false}
${"linear-system"} | ${false}
${"segment"} | ${false}
${"circle"} | ${false}
${"quadratic"} | ${false}
${"sinusoid"} | ${false}
${"polygon"} | ${true}
${"angle"} | ${false}
${"point"} | ${false}
`(
"should render for $type graphs if polygon flag is on: $shouldRender",
async ({type, shouldRender}) => {
// Arrange

// Act
Comment on lines +868 to +870
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: Is it best practice to still have both of these (arrange and act) and to have them separate like this if arrange is empty? Just curious as I usually combine them and want to make sure I do it the best way 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like there's no style guide for this, but there's a preference among folks to leave the comments on separate lines.

https://khanacademy.slack.com/archives/CJSB0D3BN/p1722962522758789

render(
<InteractiveGraphEditor
{...baseProps}
apiOptions={{
...ApiOptions.defaults,
flags: {
...flags,
mafs: {
...flags.mafs,
"start-coords-ui-phase-1": false,
"start-coords-ui-phase-2": false,
"start-coords-ui-point": false,
"start-coords-ui-polygon": true,
},
},
}}
Expand Down
5 changes: 4 additions & 1 deletion packages/perseus-editor/src/widgets/interactive-graph-editor.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -354,7 +354,10 @@ class InteractiveGraphEditor extends React.Component<Props> {
coords: null,
};

this.props.onChange({correct: graph});
this.props.onChange({
correct: graph,
graph: graph,
});
}}
style={styles.singleSelectShort}
>
Expand Down
1 change: 1 addition & 0 deletions packages/perseus/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@ export {
getLineCoords,
getLinearSystemCoords,
getPointCoords,
getPolygonCoords,
getSegmentCoords,
getSinusoidCoords,
getQuadraticCoords,
Expand Down
5 changes: 5 additions & 0 deletions packages/perseus/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,11 @@ export const InteractiveGraphEditorFlags = [
* Includes point graph.
*/
"start-coords-ui-point",
/**
* Enables the UI for setting the start coordinates of a graph.
* Includes polygon graph.
*/
"start-coords-ui-polygon",
] as const;

/**
Expand Down
2 changes: 1 addition & 1 deletion packages/perseus/src/widgets/interactive-graphs/reducer/initialize-graph-state.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,7 @@ export function getLinearSystemCoords(
);
}

function getPolygonCoords(
export function getPolygonCoords(
graph: PerseusGraphTypePolygon,
range: [x: Interval, y: Interval],
step: [x: number, y: number],
Expand Down