Skip to content

Commit

Permalink
[Interactive Graph Editor] Update the locked ellipse settings so they…
Browse files Browse the repository at this point in the history 
… only take degrees for the angle input (#1348)

## Summary:
We had it so that locked ellipse settings allow for either degrees or radians
for the angel input. However, I realized that we're not saving the data for
what unit is being used. This means that if an angle were saved as "30 degrees"
and then the page were reloaded, it would then show up as "0.523599 radians".

For the sake of simplicty and consistency, I'm changing this so that it only
takes degrees, and I'm changing it to a number input since it no longer needs
to evaluate expressions with pi (that required it being a text input).

Issue: https://khanacademy.atlassian.net/browse/LEMS-1941

## Test plan:
`yarn jest`

Storybook
- http://localhost:6006/?path=/story/perseuseditor-editorpage--mafs-with-locked-figures-m-2-flag
- Confirm that the ellipse settings angle input shows degrees only
- Confirm that it is rotated correctly

<img width="386" alt="Screenshot 2024-06-13 at 2 53 47 PM" src="https://github.com/Khan/perseus/assets/13231763/2952196e-a450-4dae-b489-31d0b6ff7857">

Author: nishasy

Reviewers: nishasy, benchristel, Myranae, mark-fitzgerald

Required Reviewers:

Approved By: benchristel

Checks: ✅ Upload Coverage (ubuntu-latest, 20.x), ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Jest Coverage (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ gerald

Pull Request URL: #1348
  • Loading branch information
@nishasy
nishasy committed Jun 17, 2024
1 parent e73373f commit 73ba4f7
Show file tree
Hide file tree
Showing 8 changed files with 96 additions and 162 deletions.
6 changes: 6 additions & 0 deletions .changeset/healthy-peas-sin.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"@khanacademy/perseus": minor
"@khanacademy/perseus-editor": minor
---

[Interactive Graph Editor] Update the locked ellipse settings so they only take degrees as input.
71 changes: 16 additions & 55 deletions packages/perseus-editor/src/components/__tests__/angle-input.test.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,91 +15,52 @@ describe("AngleInput", () => {
});
});

test("calls onChange with new angle (radians)", async () => {
test("displays angle in degrees", () => {
// Arrange
const onChangeProps = jest.fn();
render(<AngleInput angle={0} onChange={onChangeProps} />, {
render(<AngleInput angle={Math.PI / 4} onChange={() => {}} />, {
wrapper: RenderStateRoot,
});

// Act
const angleInput = screen.getByLabelText("angle");
await userEvent.type(angleInput, "2pi");

// Assert
expect(onChangeProps).toHaveBeenLastCalledWith(2 * Math.PI);
// Called with "2" and "2pi", not with "2p".
expect(onChangeProps).toHaveBeenCalledTimes(2);
});

test("calls onChange with new angle (degrees)", async () => {
// Arrange
const onChangeProps = jest.fn();
render(<AngleInput angle={0} onChange={onChangeProps} />, {
wrapper: RenderStateRoot,
const angleInput = screen.getByRole("spinbutton", {
name: "angle (degrees)",
});

// Act
const angleSwitch = screen.getByRole("switch");
await userEvent.click(angleSwitch);
const angleInput = screen.getByLabelText("angle");
await userEvent.type(angleInput, "180");

// Assert
expect(onChangeProps).toHaveBeenLastCalledWith(Math.PI);
// Called with the switch, then with "1", "18", and "180".
expect(onChangeProps).toHaveBeenCalledTimes(4);
expect(angleInput).toHaveValue(45);
});

test("does not call onChange with invalid expression", async () => {
test("calls onChange with new angle in radians", async () => {
// Arrange
const onChangeProps = jest.fn();
render(<AngleInput angle={0} onChange={onChangeProps} />, {
wrapper: RenderStateRoot,
});

// Act
const angleInput = screen.getByLabelText("angle");
await userEvent.type(angleInput, "2pi +");

// Assert
// Called with "2", "2pi", and "2pi ", but
// not with "2p" or "2pi +".
expect(onChangeProps).toHaveBeenCalledTimes(3);
expect(onChangeProps).toHaveBeenLastCalledWith(2 * Math.PI);
});

test("calls onChange in radians when switched to degrees", async () => {
// Arrange
const onChangeProps = jest.fn();
render(<AngleInput angle={0} onChange={onChangeProps} />, {
wrapper: RenderStateRoot,
const angleInput = screen.getByRole("spinbutton", {
name: "angle (degrees)",
});

// Act
const angleInput = screen.getByLabelText("angle");
await userEvent.type(angleInput, "180");
const angleSwitch = screen.getByRole("switch");
await userEvent.click(angleSwitch);
await userEvent.type(angleInput, "90");

// Assert
expect(onChangeProps).toHaveBeenLastCalledWith(Math.PI);
expect(onChangeProps).toHaveBeenLastCalledWith(Math.PI / 2);
});

test("calls onChange in radians when switched to from degrees to radians", async () => {
test("does not call onChange with invalid expression", async () => {
// Arrange
const onChangeProps = jest.fn();
render(<AngleInput angle={0} onChange={onChangeProps} />, {
wrapper: RenderStateRoot,
});

// Act
const angleInput = screen.getByLabelText("angle");
await userEvent.type(angleInput, "360");
const angleSwitch = screen.getByRole("switch");
await userEvent.click(angleSwitch);
const angleInput = screen.getByRole("spinbutton", {
name: "angle (degrees)",
});
await userEvent.type(angleInput, "-");

// Assert
expect(onChangeProps).toHaveBeenLastCalledWith(2 * Math.PI);
expect(onChangeProps).not.toHaveBeenCalled();
});
});
40 changes: 39 additions & 1 deletion packages/perseus-editor/src/components/__tests__/util.test.ts
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import {getDefaultFigureForType} from "../util";
import {degreeToRadian, getDefaultFigureForType, radianToDegree} from "../util";

describe("getDefaultFigureForType", () => {
test("should return a point with default values", () => {
Expand Down Expand Up @@ -62,3 +62,41 @@ describe("getDefaultFigureForType", () => {
});
});
});

describe("degreeToRadian", () => {
test.each`
degrees | radians
${0} | ${0}
${45} | ${Math.PI / 4}
${90} | ${Math.PI / 2}
${180} | ${Math.PI}
${270} | ${Math.PI * 1.5}
${360} | ${Math.PI * 2}
${-45} | ${-Math.PI / 4}
${-90} | ${-Math.PI / 2}
`(
"should convert $degrees degrees to $radians radians",
({degrees, radians}) => {
expect(degreeToRadian(degrees)).toBe(radians);
},
);
});

describe("radianToDegree", () => {
test.each`
radians | degrees
${0} | ${0}
${Math.PI / 4} | ${45}
${Math.PI / 2} | ${90}
${Math.PI} | ${180}
${Math.PI * 1.5} | ${270}
${Math.PI * 2} | ${360}
${-Math.PI / 4} | ${-45}
${-Math.PI / 2} | ${-90}
`(
"should convert $radians radians to $degrees degrees",
({radians, degrees}) => {
expect(radianToDegree(radians)).toBe(degrees);
},
);
});
93 changes: 22 additions & 71 deletions packages/perseus-editor/src/components/angle-input.tsx
View file
Original file line number Diff line number Diff line change
@@ -1,19 +1,11 @@
import * as KAS from "@khanacademy/kas";
import {components} from "@khanacademy/perseus";
import {View} from "@khanacademy/wonder-blocks-core";
import {TextField} from "@khanacademy/wonder-blocks-form";
import {Strut} from "@khanacademy/wonder-blocks-layout";
import Switch from "@khanacademy/wonder-blocks-switch";
import {spacing} from "@khanacademy/wonder-blocks-tokens";
import {LabelMedium, LabelSmall} from "@khanacademy/wonder-blocks-typography";
import {LabelMedium} from "@khanacademy/wonder-blocks-typography";
import {StyleSheet} from "aphrodite";
import * as React from "react";

const {InfoTip} = components;

const degreeToRadian = (degrees: number) => {
return (degrees / 180) * Math.PI;
};
import {degreeToRadian, radianToDegree} from "./util";

type Props = {
angle: number;
Expand All @@ -23,73 +15,36 @@ type Props = {
const AngleInput = (props: Props) => {
const {angle, onChange} = props;

const [angleInput, setAngleInput] = React.useState(angle.toString());
const [isInDegrees, setIsInDegrees] = React.useState(false);
const [angleInput, setAngleInput] = React.useState(
radianToDegree(angle).toString(),
);

function handleAngleChange(newValue, useDegrees = isInDegrees) {
function handleAngleChange(newValue) {
// Update the local state (update the input field value).
setAngleInput(newValue);

try {
// If the new value is a valid expression, update the props.
// Save the angle in radians.
const evaluatedAngle = KAS.parse(newValue).expr.eval();

if (useDegrees) {
onChange(degreeToRadian(evaluatedAngle));
} else {
onChange(evaluatedAngle);
}
} catch (e) {
// The user likely has not finished typing the expression.
// Do nothing.
// If the new value is not a number, don't update the props.
// If it's empty, keep the props the same value instead of setting to 0.
if (isNaN(+newValue) || newValue === "") {
return;
}
}

function handleAngleTypeChange() {
// Change the angle based on the new angle type.
handleAngleChange(angleInput, !isInDegrees);

// Update the angle to the new type.
setIsInDegrees((usingDegrees) => !usingDegrees);
// Update the graph.
onChange(degreeToRadian(newValue));
}

return (
<View style={[styles.row, styles.spaceUnder]}>
{/* Label */}
<LabelMedium tag="label" style={styles.row}>
angle
<Strut size={spacing.xxSmall_6} />
<TextField
value={angleInput}
onChange={handleAngleChange}
style={styles.textField}
/>
</LabelMedium>

{/* Spacing */}
<LabelMedium tag="label" style={styles.row}>
angle (degrees)
<Strut size={spacing.xxSmall_6} />

{/* Radian/Degree Toggle */}
<LabelSmall>radians</LabelSmall>
<View style={styles.switch}>
<Switch
onChange={handleAngleTypeChange}
checked={isInDegrees}
/>
</View>
<LabelSmall>degrees</LabelSmall>

{/* Info Tooltip */}
<InfoTip>
<p>
The angle of rotation for the ellipse (if the x radius and y
radius are different).
</p>
<p>Expressions will be evaluted (e.g. "pi/2" or "5pi/4").</p>
</InfoTip>
</View>
<TextField
type="number"
value={angleInput}
onChange={handleAngleChange}
style={styles.textField}
/>
<Strut size={spacing.xxSmall_6} />
</LabelMedium>
);
};

Expand All @@ -99,12 +54,8 @@ const styles = StyleSheet.create({
flexDirection: "row",
alignItems: "center",
},
switch: {
marginLeft: spacing.xxSmall_6,
marginRight: spacing.xxSmall_6,
},
textField: {
maxWidth: spacing.xxxLarge_64,
width: spacing.xxxLarge_64,
},
});

Expand Down
1 change: 1 addition & 0 deletions packages/perseus-editor/src/components/locked-line-settings.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,7 @@ const LockedLineSettings = (props: Props) => {

const styles = StyleSheet.create({
row: {
display: "flex",
flexDirection: "row",
alignItems: "center",
},
Expand Down
7 changes: 7 additions & 0 deletions packages/perseus-editor/src/components/util.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,3 +103,10 @@ export function getDefaultFigureForType(type: LockedFigureType): LockedFigure {
throw new UnreachableCaseError(type);
}
}

export function degreeToRadian(degrees: number) {
return (degrees / 180) * Math.PI;
}
export function radianToDegree(radians: number) {
return (radians / Math.PI) * 180;
}
38 changes: 4 additions & 34 deletions ...ges/perseus-editor/src/widgets/__tests__/interactive-graph-editor-locked-figures.test.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -672,41 +672,11 @@ describe("InteractiveGraphEditor locked figures", () => {
});

// Act
const angleInput = screen.getByLabelText("angle");
await userEvent.clear(angleInput);
await userEvent.type(angleInput, "1");
await userEvent.tab();

// Assert
expect(onChangeMock).toBeCalledWith(
expect.objectContaining({
lockedFigures: [
expect.objectContaining({
type: "ellipse",
angle: 1,
}),
],
}),
);
});

test("Calls onChange when a locked ellipse's angle is changed (degrees)", async () => {
// Arrange
const onChangeMock = jest.fn();

renderEditor({
onChange: onChangeMock,
lockedFigures: [getDefaultFigureForType("ellipse")],
const angleInput = screen.getByRole("spinbutton", {
name: "angle (degrees)",
});

// Act
// Switch to degrees first
const angleUnitsSwitch = screen.getByRole("switch");
await userEvent.click(angleUnitsSwitch);

const angleInput = screen.getByLabelText("angle");
await userEvent.clear(angleInput);
await userEvent.type(angleInput, "90");
await userEvent.type(angleInput, "30");
await userEvent.tab();

// Assert
Expand All @@ -715,7 +685,7 @@ describe("InteractiveGraphEditor locked figures", () => {
lockedFigures: [
expect.objectContaining({
type: "ellipse",
angle: Math.PI / 2,
angle: Math.PI / 6,
}),
],
}),
Expand Down
2 changes: 1 addition & 1 deletion packages/perseus/src/widgets/__testdata__/interactive-graph.testdata.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2093,7 +2093,7 @@ export const segmentWithLockedFigures: PerseusRenderer =
interactiveGraphQuestionBuilder()
.addLockedPointAt(-7, -7)
.addLockedLine([-7, -5], [2, -3])
.addLockedEllipse([0, 5], [2, 2])
.addLockedEllipse([0, 5], [4, 2], {angle: Math.PI / 4})
.build();

export const segmentWithLockedEllipses: PerseusRenderer =
Expand Down

0 comments on commit 73ba4f7

Please sign in to comment.