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.

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

Add unlimited points to graph #1529

Merged
merged 19 commits into from
Aug 22, 2024
Merged
5 changes: 5 additions & 0 deletions .changeset/big-ties-fail.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@khanacademy/perseus": minor
---

Adds unlimited point graph
2 changes: 2 additions & 0 deletions packages/perseus/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,8 @@ export const MafsGraphTypeFlags = [
"sinusoid",
/** Enables the `point` interactive-graph type with a fixed number of points. */
"point",
/** Enable the `unlimited-point` interactive graph type */
"unlimited-point",
] as const;

export const InteractiveGraphLockedFeaturesFlags = [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,7 @@ describe("a mafs graph", () => {
circle: circleQuestion,
quadratic: quadraticQuestion,
sinusoid: sinusoidQuestion,
"unlimited-point": pointQuestion,
};

const graphQuestionRenderersCorrect: {
Expand All @@ -196,6 +197,7 @@ describe("a mafs graph", () => {
circle: circleQuestionWithDefaultCorrect,
quadratic: quadraticQuestionWithDefaultCorrect,
sinusoid: sinusoidQuestionWithDefaultCorrect,
"unlimited-point": pointQuestionWithDefaultCorrect,
};

describe.each(Object.entries(graphQuestionRenderers))(
Expand Down
5 changes: 1 addition & 4 deletions packages/perseus/src/widgets/interactive-graph.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2666,10 +2666,7 @@ export function shouldUseMafs(
switch (graph.type) {
case "point":
if (graph.numPoints === UNLIMITED) {
// TODO(benchristel): add a feature flag for the "unlimited"
// case once we've implemented point graphs with unlimited
// points
return false;
return Boolean(mafsFlags["unlimited-point"]);
}
return Boolean(mafsFlags["point"]);
case "polygon":
Expand Down
71 changes: 70 additions & 1 deletion packages/perseus/src/widgets/interactive-graphs/graphs/point.tsx
Copy link
Member

Choose a reason for hiding this comment

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

Most of the code in this file is now only used in the unlimited points case. That makes me think that maybe the unlimited point graph should be a separate component. What do you think?

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 broke them out into two components. I think it's easier to read now

Original file line number Diff line number Diff line change
@@ -1,17 +1,77 @@
import * as React from "react";

import {actions} from "../reducer/interactive-graph-action";
import useGraphConfig from "../reducer/use-graph-config";

import {MovablePoint} from "./components/movable-point";
import {
useTransformDimensionsToPixels,
useTransformVectorsToPixels,
pixelsToVectors,
} from "./use-transform";

import type {PointGraphState, MafsGraphProps} from "../types";

type PointGraphProps = MafsGraphProps<PointGraphState>;

export function PointGraph(props: PointGraphProps) {
export function LimitedPointGraph(props: PointGraphProps) {
const {dispatch} = props;

return (
<>
{props.graphState.coords.map((point, i) => (
<MovablePoint
key={i}
point={point}
onMove={(destination) =>
dispatch(actions.pointGraph.movePoint(i, destination))
}
/>
))}
</>
);
}

export function UnlimitedPointGraph(props: PointGraphProps) {
const {dispatch} = props;
const graphState = useGraphConfig();
const {
range: [[minX, maxX], [minY, maxY]],
} = graphState;
const width = maxX - minX;
const height = maxY - minY;
const [[widthPx, heightPx]] = useTransformDimensionsToPixels([
width,
height,
]);
const [[left, top]] = useTransformVectorsToPixels([minX, maxY]);
return (
<>
{/* This rect is here to grab clicks so that new points can be added */}
{/* It's important because it stops mouse events from propogating
when dragging a points around */}
<rect
style={{
fill: "rgba(0,0,0,0)",
}}
width={widthPx}
height={heightPx}
x={left}
y={top}
onClick={(event) => {
const elementRect =
event.currentTarget.getBoundingClientRect();

const x = event.clientX - elementRect.x;
const y = event.clientY - elementRect.y;

const graphCoordinates = pixelsToVectors(
[[x, y]],
graphState,
);
dispatch(actions.pointGraph.addPoint(graphCoordinates[0]));
}}

Check warning on line 73 in packages/perseus/src/widgets/interactive-graphs/graphs/point.tsx

View check run for this annotation

Codecov / codecov/patch

packages/perseus/src/widgets/interactive-graphs/graphs/point.tsx#L62-L73

Added lines #L62 - L73 were not covered by tests
/>
{props.graphState.coords.map((point, i) => (
<MovablePoint
key={i}
Expand All @@ -24,3 +84,12 @@
</>
);
}

export function PointGraph(props: PointGraphProps) {
const numPoints = props.graphState.numPoints;
if (numPoints === "unlimited") {
return UnlimitedPointGraph(props);
}

return LimitedPointGraph(props);
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import {
dimensionsToPixels,
pixelsToVectors,
pointToPixel,
vectorsToPixels,
} from "./use-transform";
Expand Down Expand Up @@ -133,3 +134,81 @@ describe("pointToPixel", () => {
expect(pointToPixel([1, 1], testContext)).toEqual([20, 180]);
});
});

describe("pixelsToVectors", () => {
it("transforms (0, 0) to the top left corner of the graph bounds", () => {
const testContext: GraphDimensions = {
range: [
[-3, 10],
[1, 7],
],
width: 200,
height: 200,
};
const [[x, y]] = pixelsToVectors([[0, 0]], testContext);
expect(x).toBe(-3);
expect(y).toBe(7);
});

it("transforms (0, 200) to the bottom left corner of the graph bounds", () => {
const testContext: GraphDimensions = {
range: [
[-3, 10],
[1, 7],
],
width: 200,
height: 200,
};
const [[x, y]] = pixelsToVectors([[0, 200]], testContext);
expect(x).toBe(-3);
expect(y).toBe(1);
});

it("transforms (200, 0) to the top right corner of the graph bounds", () => {
const testContext: GraphDimensions = {
range: [
[-3, 10],
[1, 7],
],
width: 200,
height: 200,
};
const [[x, y]] = pixelsToVectors([[200, 0]], testContext);
expect(x).toBe(10);
expect(y).toBe(7);
});

it("transforms (200, 200) to the bottom right corner of the graph bounds", () => {
const testContext: GraphDimensions = {
range: [
[-3, 10],
[1, 7],
],
width: 200,
height: 200,
};
const [[x, y]] = pixelsToVectors([[200, 200]], testContext);
expect(x).toBe(10);
expect(y).toBe(1);
});

it("transforms multiple vectors", () => {
const testContext: GraphDimensions = {
range: [
[-3, 10],
[1, 7],
],
width: 200,
height: 200,
};
const [a, b] = pixelsToVectors(
[
[200, 200],
[0, 0],
],
testContext,
);
expect(a).toEqual([10, 1]);
expect(b).toEqual([-3, 7]);
});
});
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import {vec} from "mafs";

import {X, Y} from "../math";
import useGraphConfig from "../reducer/use-graph-config";

import type {GraphDimensions} from "../types";
Expand Down Expand Up @@ -51,3 +52,24 @@
const graphState = useGraphConfig();
return dimensionsToPixels(dimens, graphState);
};

export function pixelsToVectors(
pixels: vec.Vector2[],
graphState: GraphDimensions,
): vec.Vector2[] {
const [[xMin, xMax], [yMin, yMax]] = graphState.range;
const {width, height} = graphState;
const xSpan = xMax - xMin;
const ySpan = yMax - yMin;

return pixels.map((pixel): vec.Vector2 => {
const x = (pixel[X] / width) * xSpan + xMin;
const y = yMax - (pixel[Y] / height) * ySpan;
return [x, y];
});
}

export const useTransformPixelsToVectors = (...pixels: vec.Vector2[]) => {
const graphState = useGraphConfig();
return pixelsToVectors(pixels, graphState);
};

Check warning on line 75 in packages/perseus/src/widgets/interactive-graphs/graphs/use-transform.ts

View check run for this annotation

Codecov / codecov/patch

packages/perseus/src/widgets/interactive-graphs/graphs/use-transform.ts#L73-L75

Added lines #L73 - L75 were not covered by tests
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import {testDependencies} from "../../../../../testing/test-dependencies";
import * as Dependencies from "../../dependencies";

import {MafsGraph} from "./mafs-graph";
import {actions} from "./reducer/interactive-graph-action";
import {actions, ADD_POINT} from "./reducer/interactive-graph-action";
import {interactiveGraphReducer} from "./reducer/interactive-graph-reducer";

import type {MafsGraphProps} from "./mafs-graph";
Expand Down Expand Up @@ -442,6 +442,7 @@ describe("MafsGraph", () => {
const mockDispatch = jest.fn();
const state: InteractiveGraphState = {
type: "point",
numPoints: 2,
hasBeenInteractedWith: true,
range: [
[-10, 10],
Expand Down Expand Up @@ -688,4 +689,77 @@ describe("MafsGraph", () => {
);
expect(state.coords).toEqual(expectedCoords);
});

describe("with an unlimited-point graph", () => {
it("displays an add point button", async () => {
// Arrange
// Render the question
const mockDispatch = jest.fn();
const state: InteractiveGraphState = {
type: "point",
numPoints: "unlimited",
hasBeenInteractedWith: true,
range: [
[-10, 10],
[-10, 10],
],
snapStep: [2, 2],
coords: [],
};

const baseMafsGraphProps = getBaseMafsGraphProps();

render(
<MafsGraph
{...baseMafsGraphProps}
state={state}
dispatch={mockDispatch}
/>,
);

// Act: NOTHING

// Assert
// Make sure the button is on the page
const addPointButton = await screen.findByText("Add Point");
expect(addPointButton).not.toBeNull();
});
it("adds a point when the add point button is clicked", async () => {
// Arrange
// Render the question
const mockDispatch = jest.fn();
const state: InteractiveGraphState = {
type: "point",
numPoints: "unlimited",
hasBeenInteractedWith: true,
range: [
[-10, 10],
[-10, 10],
],
snapStep: [2, 2],
coords: [],
};

const baseMafsGraphProps: MafsGraphProps = {
...getBaseMafsGraphProps(),
markings: "none",
};

render(
<MafsGraph
{...baseMafsGraphProps}
state={state}
dispatch={mockDispatch}
/>,
);

// Act: Click the button
const addPointButton = await screen.findByText("Add Point");
await userEvent.click(addPointButton);

expect(mockDispatch.mock.calls).toEqual([
[{type: ADD_POINT, location: [0, 0]}],
]);
});
});
});
Loading
Loading