Skip to content

Commit c72166c

Browse files
nishasynishasy
authored
[editor-pi] [Interactive Graph] Support tick labels that are multiples of pi (#2167)
## Summary: Currently, if a graph has tick labels that are multiples of pi, such as in a sinusoid graph, content authors have to set the graph markings to "none" and load in a graphie background image that has the graph with the desired tick markings. This is not only inconvenient for content authors, it also means that the screen reader will not read the correct coordinates for these interactive graphs. It might say "2" when it should be saying "2 pi." Implementing a fix for this to improve the lives of our content authors and our screen reader users. - Editor - Check if the value loaded into NumberInput is a multiple of pi, and not already written in the "pi" format. - If so, and if the `allowPiTrunacation` prop is true, divide this number by pi and append "π" to the value. - This will make it so that the value shown in the text input will show as pi format even if loaded in without it. (6.28... --> "2π") - Mafs graph - In the AxisTicks component, check if the tick step is a multiple of pi. - If so, divide all the ticks by pi and append pi to the value (special cases for 0, 1π, and -1π). - This will result in the ticks showing as integers * pi. Issue: https://khanacademy.atlassian.net/browse/LEMS-2059 ## Test plan: `yarn jest packages/perseus/src/components/__tests__/number-input.test.tsx` `yarn jest packages/perseus/src/util/math-utils.test.ts` `yarn jest packages/perseus/src/widgets/interactive-graphs/backgrounds/axis-ticks.test.ts` `yarn jest packages/perseus/src/widgets/interactive-graphs/interactive-graph.test.tsx` Storybook - http://localhost:6006/?path=/story/perseuseditor-widgets-interactive-graph--interactive-graph-sinusoid <img width="847" alt="image" src="https://github.com/user-attachments/assets/cb2044df-d8c2-496e-b78c-2ee7c6643b7d" /> Author: nishasy Reviewers: SonicScrewdriver, nishasy, catandthemachines Required Reviewers: Approved By: SonicScrewdriver Checks: ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x) Pull Request URL: #2167
1 parent 7818343 commit c72166c

File tree

12 files changed

+271
-4
lines changed

12 files changed

+271
-4
lines changed

‎.changeset/honest-parents-press.md

+6
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
---
2+
"@khanacademy/perseus": minor
3+
"@khanacademy/perseus-editor": minor
4+
---
5+
6+
[Interactive Graph] Allow axis tick labels to be multiples of pi

‎packages/perseus-editor/src/widgets/interactive-graph-editor/components/interactive-graph-settings.tsx

+5
Original file line numberDiff line numberDiff line change
@@ -495,6 +495,7 @@ class InteractiveGraphSettings extends React.Component<Props, State> {
495495
onChange={(vals) =>
496496
this.changeRange(0, vals)
497497
}
498+
allowPiTruncation={true}
498499
/>
499500
</LabeledRow>
500501
</div>
@@ -505,6 +506,7 @@ class InteractiveGraphSettings extends React.Component<Props, State> {
505506
onChange={(vals) =>
506507
this.changeRange(1, vals)
507508
}
509+
allowPiTruncation={true}
508510
/>
509511
</LabeledRow>
510512
</div>
@@ -515,6 +517,7 @@ class InteractiveGraphSettings extends React.Component<Props, State> {
515517
<RangeInput
516518
value={this.state.stepTextbox}
517519
onChange={this.changeStep}
520+
allowPiTruncation={true}
518521
/>
519522
</LabeledRow>
520523
</div>
@@ -523,6 +526,7 @@ class InteractiveGraphSettings extends React.Component<Props, State> {
523526
<RangeInput
524527
value={this.state.gridStepTextbox}
525528
onChange={this.changeGridStep}
529+
allowPiTruncation={true}
526530
/>
527531
</LabeledRow>
528532
</div>
@@ -533,6 +537,7 @@ class InteractiveGraphSettings extends React.Component<Props, State> {
533537
<RangeInput
534538
value={this.state.snapStepTextbox}
535539
onChange={this.changeSnapStep}
540+
allowPiTruncation={true}
536541
/>
537542
</LabeledRow>
538543
</div>

‎packages/perseus/src/components/__tests__/number-input.test.tsx

+43
Original file line numberDiff line numberDiff line change
@@ -111,4 +111,47 @@ describe("NumberInput", function () {
111111

112112
expect(onChange).not.toHaveBeenCalled();
113113
});
114+
115+
it.each`
116+
value | expected
117+
${Math.PI} | ${"π"}
118+
${Math.PI * 2} | ${"2π"}
119+
${Math.PI * 0.5} | ${"π/2"}
120+
`(
121+
"allowPiTruncation: loads $value as $expected on mount",
122+
async function ({value, expected}) {
123+
// Arrange
124+
125+
// Act
126+
render(
127+
<NumberInput
128+
value={value}
129+
onChange={jest.fn()}
130+
allowPiTruncation={true}
131+
/>,
132+
);
133+
const input = screen.getByRole("textbox");
134+
135+
// Assert
136+
expect(input).toHaveValue(expected);
137+
},
138+
);
139+
140+
it("does not auto-convert number to pi format when allowPiTruncation is false", async function () {
141+
// Arrange
142+
143+
// Act
144+
render(
145+
<NumberInput
146+
value={Math.PI}
147+
onChange={jest.fn()}
148+
allowPiTruncation={false}
149+
/>,
150+
);
151+
const input = screen.getByRole("textbox");
152+
153+
// Assert
154+
expect(input).not.toHaveValue("π");
155+
expect(input).toHaveValue(Math.PI.toString());
156+
});
114157
});

‎packages/perseus/src/components/number-input.tsx

+14
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import * as React from "react";
66
import _ from "underscore";
77

88
import Util from "../util";
9+
import {isPiMultiple} from "../util/math-utils";
910

1011
import {PerseusI18nContext} from "./i18n-context";
1112

@@ -48,6 +49,7 @@ class NumberInput extends React.Component<any, any> {
4849
checkValidity: PropTypes.func,
4950
size: PropTypes.oneOf(["mini", "small", "normal"]),
5051
label: PropTypes.oneOf(["put your labels outside your inputs!"]),
52+
allowPiTruncation: PropTypes.bool,
5153
};
5254

5355
static defaultProps: any = {
@@ -63,6 +65,18 @@ class NumberInput extends React.Component<any, any> {
6365
format: this.props.format,
6466
};
6567

68+
componentDidMount() {
69+
// If the value is a multiple of pi, but it is not in the pi format,
70+
// then convert it to the pi format and show it as a multiple of pi.
71+
const value = this.getValue();
72+
if (this.props.allowPiTruncation && value !== null && value !== 0) {
73+
if (this.state.format !== "pi" && isPiMultiple(value)) {
74+
this._setValue(value / Math.PI, "pi");
75+
this.setState({format: "pi"});
76+
}
77+
}
78+
}
79+
6680
componentDidUpdate(prevProps: any) {
6781
if (!knumber.equal(this.getValue(), this.props.value)) {
6882
this._setValue(this.props.value, this.state.format);

‎packages/perseus/src/components/range-input.tsx

+3
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ class RangeInput extends React.Component<any> {
1515
onChange: PropTypes.func.isRequired,
1616
placeholder: PropTypes.array,
1717
checkValidity: PropTypes.func,
18+
allowPiTruncation: PropTypes.bool,
1819
};
1920

2021
static defaultProps: any = {
@@ -43,6 +44,7 @@ class RangeInput extends React.Component<any> {
4344
// eslint-disable-next-line react/jsx-no-bind
4445
onChange={this.onChange.bind(this, 0)}
4546
placeholder={this.props.placeholder[0]}
47+
allowPiTruncation={this.props.allowPiTruncation}
4648
/>
4749
<NumberInput
4850
{...this.props}
@@ -51,6 +53,7 @@ class RangeInput extends React.Component<any> {
5153
// eslint-disable-next-line react/jsx-no-bind
5254
onChange={this.onChange.bind(this, 1)}
5355
placeholder={this.props.placeholder[1]}
56+
allowPiTruncation={this.props.allowPiTruncation}
5457
/>
5558
</div>
5659
);

‎packages/perseus/src/util/math-utils.test.ts

+32
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
import {isPiMultiple} from "./math-utils";
2+
3+
describe("isPiMultiple", () => {
4+
test.each`
5+
case | number
6+
${"π"} | ${Math.PI}
7+
${"2π"} | ${Math.PI * 2}
8+
${"3π"} | ${Math.PI * 3}
9+
${"-π"} | ${Math.PI * -1}
10+
${"-2π"} | ${Math.PI * -2}
11+
${"π/2"} | ${Math.PI / 2}
12+
${"π/3"} | ${Math.PI / 3}
13+
${"π/4"} | ${Math.PI / 4}
14+
${"π/6"} | ${Math.PI / 6}
15+
${"2π/3"} | ${(Math.PI * 2) / 3}
16+
`("should return true for $case", ({number}) => {
17+
expect(isPiMultiple(number)).toBe(true);
18+
});
19+
20+
test.each`
21+
case | number
22+
${"0"} | ${0}
23+
${"1"} | ${1}
24+
${"-1"} | ${-1}
25+
${"3.14"} | ${3.14}
26+
${"3.14159"} | ${3.14159}
27+
${"2.5"} | ${2.5}
28+
${"-1.5"} | ${-1.5}
29+
`("should return false for $case", ({number}) => {
30+
expect(isPiMultiple(number)).toBe(false);
31+
});
32+
});

‎packages/perseus/src/util/math-utils.ts

+15
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
export function isPiMultiple(value: number): boolean {
2+
// Early return for integers that clearly aren't multiples
3+
// of pi to save some computation.
4+
if (Number.isInteger(value)) {
5+
return false;
6+
}
7+
8+
return (
9+
value % Math.PI === 0 ||
10+
value % (Math.PI / 2) === 0 ||
11+
value % (Math.PI / 3) === 0 ||
12+
value % (Math.PI / 4) === 0 ||
13+
value % (Math.PI / 6) === 0
14+
);
15+
}

‎packages/perseus/src/widgets/interactive-graphs/backgrounds/axis-ticks.test.ts

+22
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import {
22
generateTickLocations,
33
shouldShowLabel,
44
countSignificantDecimals,
5+
divideByAndShowPi,
56
} from "./axis-ticks";
67

78
import type {Interval} from "mafs";
@@ -76,3 +77,24 @@ describe("countSignificantDecimals", () => {
7677
},
7778
);
7879
});
80+
81+
describe("divideByAndShowPi", () => {
82+
it.each`
83+
value | expected
84+
${Math.PI} | ${"π"}
85+
${-1 * Math.PI} | ${"-π"}
86+
${0} | ${"0"}
87+
${Math.PI * 2} | ${"2π"}
88+
${Math.PI * -2} | ${"-2π"}
89+
${Math.PI / 2} | ${"0.5π"}
90+
${Math.PI / -2} | ${"-0.5π"}
91+
`("should display $expected as a multiple of pi", ({value, expected}) => {
92+
// Arrange
93+
94+
// Act
95+
const result = divideByAndShowPi(value);
96+
97+
// Assert
98+
expect(result).toBe(expected);
99+
});
100+
});

‎packages/perseus/src/widgets/interactive-graphs/backgrounds/axis-ticks.tsx

+42-3
Original file line numberDiff line numberDiff line change
@@ -14,10 +14,13 @@ const YGridTick = ({
1414
y,
1515
range,
1616
tickStep,
17+
showPi,
1718
}: {
1819
y: number;
1920
range: [Interval, Interval];
2021
tickStep: number;
22+
// Whether to show the tick label as a multiple of pi
23+
showPi: boolean;
2124
}) => {
2225
// If the graph requires out-of-bounds labels, we want to make sure to set the
2326
// coordinates to the edge of the visible range of the graph. Otherwise,
@@ -55,6 +58,8 @@ const YGridTick = ({
5558
// to hide the label at -1 on the y-axis to prevent overlap with the x-axis label
5659
const showLabel = shouldShowLabel(y, range, tickStep);
5760

61+
const yLabel = showPi ? divideByAndShowPi(y) : y.toString();
62+
5863
return (
5964
<g className="tick" aria-hidden={true}>
6065
<line x1={x1} y1={y1} x2={x2} y2={y2} className="axis-tick" />
@@ -66,14 +71,23 @@ const YGridTick = ({
6671
x={xPositionText}
6772
y={yPositionText}
6873
>
69-
{y.toString()}
74+
{yLabel}
7075
</text>
7176
)}
7277
</g>
7378
);
7479
};
7580

76-
const XGridTick = ({x, range}: {x: number; range: [Interval, Interval]}) => {
81+
const XGridTick = ({
82+
x,
83+
range,
84+
showPi,
85+
}: {
86+
x: number;
87+
range: [Interval, Interval];
88+
// Whether to show the tick label as a multiple of pi
89+
showPi: boolean;
90+
}) => {
7791
// If the graph requires out-of-bounds labels, we want to make sure to set the
7892
// coordinates to the edge of the visible range of the graph. Otherwise,
7993
// the ticks and labels would render outside of the clipping-mask.
@@ -113,6 +127,8 @@ const XGridTick = ({x, range}: {x: number; range: [Interval, Interval]}) => {
113127
const xPositionText = xPosition + xAdjustment;
114128
const yPositionText = yPosition + yAdjustment;
115129

130+
const xLabel = showPi ? divideByAndShowPi(x) : x.toString();
131+
116132
return (
117133
<g className="tick" aria-hidden={true}>
118134
<line x1={x1} y1={y1} x2={x2} y2={y2} className="axis-tick" />
@@ -124,7 +140,7 @@ const XGridTick = ({x, range}: {x: number; range: [Interval, Interval]}) => {
124140
x={xPositionText}
125141
y={yPositionText}
126142
>
127-
{x.toString()}
143+
{xLabel}
128144
</text>
129145
}
130146
</g>
@@ -190,6 +206,23 @@ export const countSignificantDecimals = (number: number): number => {
190206
return numStr.split(".")[1].length;
191207
};
192208

209+
// Show the given value as a multiple of pi (already assumed to be
210+
// a multiple of pi). Exported for testing
211+
export function divideByAndShowPi(value: number): string {
212+
const dividedValue = value / Math.PI;
213+
214+
switch (dividedValue) {
215+
case 1:
216+
return "π";
217+
case -1:
218+
return "-π";
219+
case 0:
220+
return "0";
221+
default:
222+
return dividedValue + "π";
223+
}
224+
}
225+
193226
export const AxisTicks = () => {
194227
const {tickStep, range} = useGraphConfig();
195228
const [[xMin, xMax], [yMin, yMax]] = range;
@@ -209,6 +242,9 @@ export const AxisTicks = () => {
209242
key={`y-grid-tick-${y}`}
210243
range={range}
211244
tickStep={tickStep[Y]}
245+
// Show the tick labels as multiples of pi
246+
// if the tick step is a multiple of pi.
247+
showPi={tickStep[Y] % Math.PI === 0}
212248
/>
213249
);
214250
})}
@@ -220,6 +256,9 @@ export const AxisTicks = () => {
220256
x={x}
221257
key={`x-grid-tick-${x}`}
222258
range={range}
259+
// Show the tick labels as multiples of pi
260+
// if the tick step is a multiple of pi.
261+
showPi={tickStep[X] % Math.PI === 0}
223262
/>
224263
);
225264
})}

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

+8
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import Renderer from "../../renderer";
44
import {mockStrings} from "../../strings";
55

66
import {interactiveGraphQuestionBuilder} from "./interactive-graph-question-builder";
7+
import {sinusoidWithPiTicks} from "./interactive-graph.testdata";
78

89
import type {PerseusRenderer} from "@khanacademy/perseus-core";
910
import type {Meta, StoryObj} from "@storybook/react";
@@ -249,6 +250,12 @@ export const MafsWithProtractor: Story = {
249250
},
250251
};
251252

253+
export const MafsWithPiTicks: Story = {
254+
args: {
255+
question: sinusoidWithPiTicks,
256+
},
257+
};
258+
252259
function MafsQuestionRenderer(props: {question: PerseusRenderer}) {
253260
const {question} = props;
254261
return (
@@ -263,6 +270,7 @@ function MafsQuestionRenderer(props: {question: PerseusRenderer}) {
263270
segment: true,
264271
circle: true,
265272
linear: true,
273+
sinusoid: true,
266274
},
267275
},
268276
}}

0 commit comments

Comments
 (0)