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

[Hint Mode: Start Coords] Add start coords UI for quadratic graphs #1469

Merged
merged 1 commit into from
Jul 31, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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/grumpy-rivers-draw.md
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 quadratic graphs
Original file line number Diff line number Diff line change
Expand Up @@ -455,4 +455,72 @@ describe("StartCoordSettings", () => {
},
);
});

describe("quadratic graph", () => {
test("shows the start coordinates UI", () => {
// Arrange

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

// Assert
expect(screen.getByText("Start coordinates")).toBeInTheDocument();
expect(screen.getByText("Starting equation:")).toBeInTheDocument();
expect(
// Equation for default start coords
screen.getByText("y = 0.400x^2 + 0.000x + -5.000"),
).toBeInTheDocument();
expect(screen.getByText("Point 1:")).toBeInTheDocument();
expect(screen.getByText("Point 2:")).toBeInTheDocument();
expect(screen.getByText("Point 3:")).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();

// Act
render(
<StartCoordsSettings
{...defaultProps}
type="quadratic"
onChange={onChangeMock}
/>,
);

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

const expectedCoords = [
[-5, 5],
[0, -5],
[5, 5],
];
expectedCoords[pointIndex][coord === "x" ? 0 : 1] = 101;

expect(onChangeMock).toHaveBeenLastCalledWith(expectedCoords);
},
);
});
});
32 changes: 32 additions & 0 deletions packages/perseus-editor/src/components/__tests__/util.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import {
radianToDegree,
getDefaultGraphStartCoords,
getSinusoidEquation,
getQuadraticEquation,
} from "../util";

import type {PerseusGraphType, Range} from "@khanacademy/perseus";
Expand Down Expand Up @@ -276,3 +277,34 @@ describe("getSinusoidEquation", () => {
expect(equation).toBe(expected);
});
});

describe("getQuadraticEquation", () => {
test.each`
point1 | point2 | point3 | expected
${[-5, 5]} | ${[0, -5]} | ${[5, 5]} | ${"y = 0.400x^2 + 0.000x + -5.000"}
${[-5, 5]} | ${[0, 5]} | ${[5, 5]} | ${"y = 0.000x^2 + 0.000x + 5.000"}
${[-9, 9]} | ${[-7, 7]} | ${[9, 9]} | ${"y = 0.063x^2 + 0.000x + 3.938"}
${[-9, 4]} | ${[-7, 7]} | ${[9, 9]} | ${"y = -0.076x^2 + 0.278x + 12.688"}
${[-1, 0]} | ${[0, 1]} | ${[1, 2]} | ${"y = 0.000x^2 + 1.000x + 1.000"}
`(
"should return the correct equation",
({point1, point2, point3, expected}) => {
// Act
const equation = getQuadraticEquation([point1, point2, point3]);

expect(equation).toBe(expected);
},
);

test.each`
point1 | point2 | point3
${[-5, 5]} | ${[-5, -5]} | ${[5, 5]}
${[-5, 5]} | ${[0, -5]} | ${[-5, 5]}
${[-5, 5]} | ${[0, 5]} | ${[0, 5]}
`("should return division by zero error", ({point1, point2, point3}) => {
// Act
const equation = getQuadraticEquation([point1, point2, point3]);

expect(equation).toBe("Division by zero error");
Copy link
Member

Choose a reason for hiding this comment

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

This is an interesting case! Thanks for testing it.

I am curious: how does the graph preview render in this case? What happens to the graph when the start coords aren't valid?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It just shows all the points where they would be, but the quadratic graph doesn't go through the points. The equation box says "Division by zero error"

Screenshot 2024-07-31 at 3 43 19 PM Screenshot 2024-07-31 at 3 43 33 PM

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 not too worried about it because I think this is within the "trust the content authors" territory

});
});
95 changes: 95 additions & 0 deletions packages/perseus-editor/src/components/start-coords-quadratic.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
import {View} from "@khanacademy/wonder-blocks-core";
import {Strut} from "@khanacademy/wonder-blocks-layout";
import {color, font, spacing} from "@khanacademy/wonder-blocks-tokens";
import {
BodyMonospace,
LabelLarge,
LabelMedium,
} from "@khanacademy/wonder-blocks-typography";
import {StyleSheet} from "aphrodite";
import * as React from "react";

import CoordinatePairInput from "./coordinate-pair-input";
import {getQuadraticEquation} from "./util";

import type {Coord, PerseusGraphType} from "@khanacademy/perseus";

type Props = {
startCoords: [Coord, Coord, Coord];
onChange: (startCoords: PerseusGraphType["startCoords"]) => void;
};

const StartCoordsQuadratic = (props: Props) => {
const {startCoords, onChange} = props;

return (
<>
{/* Current equation */}
<View style={styles.equationSection}>
<LabelMedium>Starting equation:</LabelMedium>
<BodyMonospace style={styles.equationBody}>
{getQuadraticEquation(startCoords)}
</BodyMonospace>
</View>

{/* Points UI */}
<View style={styles.tile}>
<LabelLarge>Point 1:</LabelLarge>
<Strut size={spacing.small_12} />
<CoordinatePairInput
coord={startCoords[0]}
labels={["x", "y"]}
onChange={(value) =>
onChange([value, startCoords[1], startCoords[2]])
}
/>
</View>
<View style={styles.tile}>
<LabelLarge>Point 2:</LabelLarge>
<Strut size={spacing.small_12} />
<CoordinatePairInput
coord={startCoords[1]}
labels={["x", "y"]}
onChange={(value) =>
onChange([startCoords[0], value, startCoords[2]])
}
/>
</View>
<View style={styles.tile}>
<LabelLarge>Point 3:</LabelLarge>
<Strut size={spacing.small_12} />
<CoordinatePairInput
coord={startCoords[2]}
labels={["x", "y"]}
onChange={(value) =>
onChange([startCoords[0], startCoords[1], value])
}
/>
</View>
</>
);
};

const styles = StyleSheet.create({
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: Grrr. Same styling as all the other "start-coords-*" implementations.
No need to change anything in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I did admittedly copy/paste the entire sinusoid file and build off of it. I should probably break out Tile into a separate component. Maybe in a different PR.

tile: {
backgroundColor: color.fadedBlue8,
marginTop: spacing.xSmall_8,
padding: spacing.small_12,
borderRadius: spacing.xSmall_8,
flexDirection: "row",
alignItems: "center",
},
equationSection: {
marginTop: spacing.small_12,
},
equationBody: {
backgroundColor: color.fadedOffBlack8,
border: `1px solid ${color.fadedOffBlack32}`,
marginTop: spacing.xSmall_8,
paddingLeft: spacing.xSmall_8,
paddingRight: spacing.xSmall_8,
fontSize: font.size.xSmall,
},
});

export default StartCoordsQuadratic;
11 changes: 10 additions & 1 deletion packages/perseus-editor/src/components/start-coords-settings.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import {
getCircleCoords,
getLineCoords,
getLinearSystemCoords,
getQuadraticCoords,
getSegmentCoords,
getSinusoidCoords,
} from "@khanacademy/perseus";
Expand All @@ -17,6 +18,7 @@ import Heading from "./heading";
import StartCoordsCircle from "./start-coords-circle";
import StartCoordsLine from "./start-coords-line";
import StartCoordsMultiline from "./start-coords-multiline";
import StartCoordsQuadratic from "./start-coords-quadratic";
import StartCoordsSinusoid from "./start-coords-sinusoid";
import {getDefaultGraphStartCoords} from "./util";

Expand Down Expand Up @@ -79,7 +81,14 @@ const StartCoordsSettingsInner = (props: Props) => {
onChange={onChange}
/>
);

case "quadratic":
const quadraticCoords = getQuadraticCoords(props, range, step);
return (
<StartCoordsQuadratic
startCoords={quadraticCoords}
onChange={onChange}
/>
);
default:
return null;
}
Expand Down
38 changes: 38 additions & 0 deletions packages/perseus-editor/src/components/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
getCircleCoords,
getLineCoords,
getLinearSystemCoords,
getQuadraticCoords,
getSegmentCoords,
getSinusoidCoords,
} from "@khanacademy/perseus";
Expand Down Expand Up @@ -187,6 +188,12 @@
range,
step,
);
case "quadratic":
return getQuadraticCoords(
{...graph, startCoords: undefined},
range,
step,
);

Check warning on line 196 in packages/perseus-editor/src/components/util.ts

View check run for this annotation

Codecov / codecov/patch

packages/perseus-editor/src/components/util.ts#L192-L196

Added lines #L192 - L196 were not covered by tests
default:
return undefined;
}
Expand Down Expand Up @@ -215,3 +222,34 @@
verticalOffset.toFixed(3)
);
};

export const getQuadraticEquation = (startCoords: [Coord, Coord, Coord]) => {
const p1 = startCoords[0];
const p2 = startCoords[1];
const p3 = startCoords[2];

const denom = (p1[0] - p2[0]) * (p1[0] - p3[0]) * (p2[0] - p3[0]);
if (denom === 0) {
// Many of the callers assume that the return value is always defined.
return "Division by zero error";
}
const a =
(p3[0] * (p2[1] - p1[1]) +
p2[0] * (p1[1] - p3[1]) +
p1[0] * (p3[1] - p2[1])) /
denom;
const b =
(p3[0] * p3[0] * (p1[1] - p2[1]) +
p2[0] * p2[0] * (p3[1] - p1[1]) +
p1[0] * p1[0] * (p2[1] - p3[1])) /
denom;
const c =
(p2[0] * p3[0] * (p2[0] - p3[0]) * p1[1] +
p3[0] * p1[0] * (p3[0] - p1[0]) * p2[1] +
p1[0] * p2[0] * (p1[0] - p2[0]) * p3[1]) /
denom;
Copy link
Member

Choose a reason for hiding this comment

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

This is copied from InteractiveGraph.getQuadraticCoefficients?

I wish we had a central place where this logic could live, and both perseus and perseus-editor could import it from there. I think kmath is probably the best candidate that place. Would it be too hard to move getQuadraticCoefficients to kmath?

If you don't want to move it in this PR, I can move it once this is landed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that would be awesome! Sorry, I didn't see your comment before landing.


return (
"y = " + a.toFixed(3) + "x^2 + " + b.toFixed(3) + "x + " + c.toFixed(3)
);
};
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ const startCoordsUiPhase1Types = [
"segment",
"circle",
];
const startCoordsUiPhase2Types = ["sinusoid"];
const startCoordsUiPhase2Types = ["sinusoid", "quadratic"];

type Range = [min: number, max: number];

Expand Down
1 change: 1 addition & 0 deletions packages/perseus/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,7 @@ export {
getLinearSystemCoords,
getSegmentCoords,
getSinusoidCoords,
getQuadraticCoords,
} from "./widgets/interactive-graphs/reducer/initialize-graph-state";

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -345,7 +345,7 @@ export function getSinusoidCoords(
return coords;
}

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