From c72166c3046aa7c0fcd4e3348d604248e8565c2e Mon Sep 17 00:00:00 2001 From: Nisha Yerunkar Date: Wed, 29 Jan 2025 16:07:37 -0800 Subject: [PATCH] [editor-pi] [Interactive Graph] Support tick labels that are multiples of pi (#2167) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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 image 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: https://github.com/Khan/perseus/pull/2167 --- .changeset/honest-parents-press.md | 6 ++ .../components/interactive-graph-settings.tsx | 5 ++ .../__tests__/number-input.test.tsx | 43 ++++++++++++ .../perseus/src/components/number-input.tsx | 14 ++++ .../perseus/src/components/range-input.tsx | 3 + packages/perseus/src/util/math-utils.test.ts | 32 +++++++++ packages/perseus/src/util/math-utils.ts | 15 +++++ .../backgrounds/axis-ticks.test.ts | 22 +++++++ .../backgrounds/axis-ticks.tsx | 45 ++++++++++++- .../interactive-graph-regression.stories.tsx | 8 +++ .../interactive-graph.test.tsx | 66 +++++++++++++++++++ .../interactive-graph.testdata.ts | 16 ++++- 12 files changed, 271 insertions(+), 4 deletions(-) create mode 100644 .changeset/honest-parents-press.md create mode 100644 packages/perseus/src/util/math-utils.test.ts create mode 100644 packages/perseus/src/util/math-utils.ts diff --git a/.changeset/honest-parents-press.md b/.changeset/honest-parents-press.md new file mode 100644 index 0000000000..ee9f58be49 --- /dev/null +++ b/.changeset/honest-parents-press.md @@ -0,0 +1,6 @@ +--- +"@khanacademy/perseus": minor +"@khanacademy/perseus-editor": minor +--- + +[Interactive Graph] Allow axis tick labels to be multiples of pi diff --git a/packages/perseus-editor/src/widgets/interactive-graph-editor/components/interactive-graph-settings.tsx b/packages/perseus-editor/src/widgets/interactive-graph-editor/components/interactive-graph-settings.tsx index 8e0158aaaa..baa32fbbb4 100644 --- a/packages/perseus-editor/src/widgets/interactive-graph-editor/components/interactive-graph-settings.tsx +++ b/packages/perseus-editor/src/widgets/interactive-graph-editor/components/interactive-graph-settings.tsx @@ -495,6 +495,7 @@ class InteractiveGraphSettings extends React.Component { onChange={(vals) => this.changeRange(0, vals) } + allowPiTruncation={true} /> @@ -505,6 +506,7 @@ class InteractiveGraphSettings extends React.Component { onChange={(vals) => this.changeRange(1, vals) } + allowPiTruncation={true} /> @@ -515,6 +517,7 @@ class InteractiveGraphSettings extends React.Component { @@ -523,6 +526,7 @@ class InteractiveGraphSettings extends React.Component { @@ -533,6 +537,7 @@ class InteractiveGraphSettings extends React.Component { diff --git a/packages/perseus/src/components/__tests__/number-input.test.tsx b/packages/perseus/src/components/__tests__/number-input.test.tsx index 45e26d6269..88583db66c 100644 --- a/packages/perseus/src/components/__tests__/number-input.test.tsx +++ b/packages/perseus/src/components/__tests__/number-input.test.tsx @@ -111,4 +111,47 @@ describe("NumberInput", function () { expect(onChange).not.toHaveBeenCalled(); }); + + it.each` + value | expected + ${Math.PI} | ${"π"} + ${Math.PI * 2} | ${"2π"} + ${Math.PI * 0.5} | ${"π/2"} + `( + "allowPiTruncation: loads $value as $expected on mount", + async function ({value, expected}) { + // Arrange + + // Act + render( + , + ); + const input = screen.getByRole("textbox"); + + // Assert + expect(input).toHaveValue(expected); + }, + ); + + it("does not auto-convert number to pi format when allowPiTruncation is false", async function () { + // Arrange + + // Act + render( + , + ); + const input = screen.getByRole("textbox"); + + // Assert + expect(input).not.toHaveValue("π"); + expect(input).toHaveValue(Math.PI.toString()); + }); }); diff --git a/packages/perseus/src/components/number-input.tsx b/packages/perseus/src/components/number-input.tsx index b618ad1b07..b336d6c346 100644 --- a/packages/perseus/src/components/number-input.tsx +++ b/packages/perseus/src/components/number-input.tsx @@ -6,6 +6,7 @@ import * as React from "react"; import _ from "underscore"; import Util from "../util"; +import {isPiMultiple} from "../util/math-utils"; import {PerseusI18nContext} from "./i18n-context"; @@ -48,6 +49,7 @@ class NumberInput extends React.Component { checkValidity: PropTypes.func, size: PropTypes.oneOf(["mini", "small", "normal"]), label: PropTypes.oneOf(["put your labels outside your inputs!"]), + allowPiTruncation: PropTypes.bool, }; static defaultProps: any = { @@ -63,6 +65,18 @@ class NumberInput extends React.Component { format: this.props.format, }; + componentDidMount() { + // If the value is a multiple of pi, but it is not in the pi format, + // then convert it to the pi format and show it as a multiple of pi. + const value = this.getValue(); + if (this.props.allowPiTruncation && value !== null && value !== 0) { + if (this.state.format !== "pi" && isPiMultiple(value)) { + this._setValue(value / Math.PI, "pi"); + this.setState({format: "pi"}); + } + } + } + componentDidUpdate(prevProps: any) { if (!knumber.equal(this.getValue(), this.props.value)) { this._setValue(this.props.value, this.state.format); diff --git a/packages/perseus/src/components/range-input.tsx b/packages/perseus/src/components/range-input.tsx index b60dcad131..cb0c565332 100644 --- a/packages/perseus/src/components/range-input.tsx +++ b/packages/perseus/src/components/range-input.tsx @@ -15,6 +15,7 @@ class RangeInput extends React.Component { onChange: PropTypes.func.isRequired, placeholder: PropTypes.array, checkValidity: PropTypes.func, + allowPiTruncation: PropTypes.bool, }; static defaultProps: any = { @@ -43,6 +44,7 @@ class RangeInput extends React.Component { // eslint-disable-next-line react/jsx-no-bind onChange={this.onChange.bind(this, 0)} placeholder={this.props.placeholder[0]} + allowPiTruncation={this.props.allowPiTruncation} /> { // eslint-disable-next-line react/jsx-no-bind onChange={this.onChange.bind(this, 1)} placeholder={this.props.placeholder[1]} + allowPiTruncation={this.props.allowPiTruncation} /> ); diff --git a/packages/perseus/src/util/math-utils.test.ts b/packages/perseus/src/util/math-utils.test.ts new file mode 100644 index 0000000000..4313e3d543 --- /dev/null +++ b/packages/perseus/src/util/math-utils.test.ts @@ -0,0 +1,32 @@ +import {isPiMultiple} from "./math-utils"; + +describe("isPiMultiple", () => { + test.each` + case | number + ${"π"} | ${Math.PI} + ${"2π"} | ${Math.PI * 2} + ${"3π"} | ${Math.PI * 3} + ${"-π"} | ${Math.PI * -1} + ${"-2π"} | ${Math.PI * -2} + ${"π/2"} | ${Math.PI / 2} + ${"π/3"} | ${Math.PI / 3} + ${"π/4"} | ${Math.PI / 4} + ${"π/6"} | ${Math.PI / 6} + ${"2π/3"} | ${(Math.PI * 2) / 3} + `("should return true for $case", ({number}) => { + expect(isPiMultiple(number)).toBe(true); + }); + + test.each` + case | number + ${"0"} | ${0} + ${"1"} | ${1} + ${"-1"} | ${-1} + ${"3.14"} | ${3.14} + ${"3.14159"} | ${3.14159} + ${"2.5"} | ${2.5} + ${"-1.5"} | ${-1.5} + `("should return false for $case", ({number}) => { + expect(isPiMultiple(number)).toBe(false); + }); +}); diff --git a/packages/perseus/src/util/math-utils.ts b/packages/perseus/src/util/math-utils.ts new file mode 100644 index 0000000000..890c06e460 --- /dev/null +++ b/packages/perseus/src/util/math-utils.ts @@ -0,0 +1,15 @@ +export function isPiMultiple(value: number): boolean { + // Early return for integers that clearly aren't multiples + // of pi to save some computation. + if (Number.isInteger(value)) { + return false; + } + + return ( + value % Math.PI === 0 || + value % (Math.PI / 2) === 0 || + value % (Math.PI / 3) === 0 || + value % (Math.PI / 4) === 0 || + value % (Math.PI / 6) === 0 + ); +} diff --git a/packages/perseus/src/widgets/interactive-graphs/backgrounds/axis-ticks.test.ts b/packages/perseus/src/widgets/interactive-graphs/backgrounds/axis-ticks.test.ts index a59b289b3a..72accd4779 100644 --- a/packages/perseus/src/widgets/interactive-graphs/backgrounds/axis-ticks.test.ts +++ b/packages/perseus/src/widgets/interactive-graphs/backgrounds/axis-ticks.test.ts @@ -2,6 +2,7 @@ import { generateTickLocations, shouldShowLabel, countSignificantDecimals, + divideByAndShowPi, } from "./axis-ticks"; import type {Interval} from "mafs"; @@ -76,3 +77,24 @@ describe("countSignificantDecimals", () => { }, ); }); + +describe("divideByAndShowPi", () => { + it.each` + value | expected + ${Math.PI} | ${"π"} + ${-1 * Math.PI} | ${"-π"} + ${0} | ${"0"} + ${Math.PI * 2} | ${"2π"} + ${Math.PI * -2} | ${"-2π"} + ${Math.PI / 2} | ${"0.5π"} + ${Math.PI / -2} | ${"-0.5π"} + `("should display $expected as a multiple of pi", ({value, expected}) => { + // Arrange + + // Act + const result = divideByAndShowPi(value); + + // Assert + expect(result).toBe(expected); + }); +}); diff --git a/packages/perseus/src/widgets/interactive-graphs/backgrounds/axis-ticks.tsx b/packages/perseus/src/widgets/interactive-graphs/backgrounds/axis-ticks.tsx index 40757be70f..a17a1a14b6 100644 --- a/packages/perseus/src/widgets/interactive-graphs/backgrounds/axis-ticks.tsx +++ b/packages/perseus/src/widgets/interactive-graphs/backgrounds/axis-ticks.tsx @@ -14,10 +14,13 @@ const YGridTick = ({ y, range, tickStep, + showPi, }: { y: number; range: [Interval, Interval]; tickStep: number; + // Whether to show the tick label as a multiple of pi + showPi: boolean; }) => { // If the graph requires out-of-bounds labels, we want to make sure to set the // coordinates to the edge of the visible range of the graph. Otherwise, @@ -55,6 +58,8 @@ const YGridTick = ({ // to hide the label at -1 on the y-axis to prevent overlap with the x-axis label const showLabel = shouldShowLabel(y, range, tickStep); + const yLabel = showPi ? divideByAndShowPi(y) : y.toString(); + return ( @@ -66,14 +71,23 @@ const YGridTick = ({ x={xPositionText} y={yPositionText} > - {y.toString()} + {yLabel} )} ); }; -const XGridTick = ({x, range}: {x: number; range: [Interval, Interval]}) => { +const XGridTick = ({ + x, + range, + showPi, +}: { + x: number; + range: [Interval, Interval]; + // Whether to show the tick label as a multiple of pi + showPi: boolean; +}) => { // If the graph requires out-of-bounds labels, we want to make sure to set the // coordinates to the edge of the visible range of the graph. Otherwise, // the ticks and labels would render outside of the clipping-mask. @@ -113,6 +127,8 @@ const XGridTick = ({x, range}: {x: number; range: [Interval, Interval]}) => { const xPositionText = xPosition + xAdjustment; const yPositionText = yPosition + yAdjustment; + const xLabel = showPi ? divideByAndShowPi(x) : x.toString(); + return ( @@ -124,7 +140,7 @@ const XGridTick = ({x, range}: {x: number; range: [Interval, Interval]}) => { x={xPositionText} y={yPositionText} > - {x.toString()} + {xLabel} } @@ -190,6 +206,23 @@ export const countSignificantDecimals = (number: number): number => { return numStr.split(".")[1].length; }; +// Show the given value as a multiple of pi (already assumed to be +// a multiple of pi). Exported for testing +export function divideByAndShowPi(value: number): string { + const dividedValue = value / Math.PI; + + switch (dividedValue) { + case 1: + return "π"; + case -1: + return "-π"; + case 0: + return "0"; + default: + return dividedValue + "π"; + } +} + export const AxisTicks = () => { const {tickStep, range} = useGraphConfig(); const [[xMin, xMax], [yMin, yMax]] = range; @@ -209,6 +242,9 @@ export const AxisTicks = () => { key={`y-grid-tick-${y}`} range={range} tickStep={tickStep[Y]} + // Show the tick labels as multiples of pi + // if the tick step is a multiple of pi. + showPi={tickStep[Y] % Math.PI === 0} /> ); })} @@ -220,6 +256,9 @@ export const AxisTicks = () => { x={x} key={`x-grid-tick-${x}`} range={range} + // Show the tick labels as multiples of pi + // if the tick step is a multiple of pi. + showPi={tickStep[X] % Math.PI === 0} /> ); })} diff --git a/packages/perseus/src/widgets/interactive-graphs/interactive-graph-regression.stories.tsx b/packages/perseus/src/widgets/interactive-graphs/interactive-graph-regression.stories.tsx index 2d6276cd84..245271e6ca 100644 --- a/packages/perseus/src/widgets/interactive-graphs/interactive-graph-regression.stories.tsx +++ b/packages/perseus/src/widgets/interactive-graphs/interactive-graph-regression.stories.tsx @@ -4,6 +4,7 @@ import Renderer from "../../renderer"; import {mockStrings} from "../../strings"; import {interactiveGraphQuestionBuilder} from "./interactive-graph-question-builder"; +import {sinusoidWithPiTicks} from "./interactive-graph.testdata"; import type {PerseusRenderer} from "@khanacademy/perseus-core"; import type {Meta, StoryObj} from "@storybook/react"; @@ -249,6 +250,12 @@ export const MafsWithProtractor: Story = { }, }; +export const MafsWithPiTicks: Story = { + args: { + question: sinusoidWithPiTicks, + }, +}; + function MafsQuestionRenderer(props: {question: PerseusRenderer}) { const {question} = props; return ( @@ -263,6 +270,7 @@ function MafsQuestionRenderer(props: {question: PerseusRenderer}) { segment: true, circle: true, linear: true, + sinusoid: true, }, }, }} diff --git a/packages/perseus/src/widgets/interactive-graphs/interactive-graph.test.tsx b/packages/perseus/src/widgets/interactive-graphs/interactive-graph.test.tsx index 3ab059b049..d7882f127e 100644 --- a/packages/perseus/src/widgets/interactive-graphs/interactive-graph.test.tsx +++ b/packages/perseus/src/widgets/interactive-graphs/interactive-graph.test.tsx @@ -57,6 +57,7 @@ import { segmentWithLockedPolygonWhite, segmentWithLockedVectors, sinusoidQuestionWithDefaultCorrect, + sinusoidWithPiTicks, } from "./interactive-graph.testdata"; import {trueForAllMafsSupportedGraphTypes} from "./mafs-supported-graph-types"; @@ -1394,4 +1395,69 @@ describe("Interactive Graph", function () { expect(graph).not.toHaveAttribute("aria-describedby"); }); }); + + describe("axis labels", () => { + test("should render x axis labels as multiples of pi if the tick step is a multiple of pi", () => { + // Arrange + const {container} = renderQuestion(sinusoidWithPiTicks, { + flags: {mafs: {sinusoid: true}}, + }); + + // Act + // eslint-disable-next-line testing-library/no-container, testing-library/no-node-access + const xAxisLabels = container.querySelectorAll( + ".x-axis-ticks > .tick", + ); + + // Assert + // The first label is π + expect(xAxisLabels[0]).toHaveTextContent("π"); + + // There are four more labels in the positive direction + for (let i = 0; i < 4; i++) { + expect(xAxisLabels[i + 1]).toHaveTextContent(`${i + 2}π`); + } + + // First label in the negative direction is -π + expect(xAxisLabels[5]).toHaveTextContent("-π"); + + // There are four more labels in the negative direction + for (let i = 0; i < 4; i++) { + expect(xAxisLabels[i + 6]).toHaveTextContent(`-${i + 2}π`); + } + }); + + test("should render y axis labels as multiples of pi if the tick step is a multiple of pi", () => { + // Arrange + const {container} = renderQuestion(sinusoidWithPiTicks, { + flags: {mafs: {sinusoid: true}}, + }); + + // Act + // eslint-disable-next-line testing-library/no-container, testing-library/no-node-access + const yAxisLabels = container.querySelectorAll( + ".y-axis-ticks > .tick", + ); + + // Assert + // The first label is 2π + expect(yAxisLabels[0]).toHaveTextContent("2π"); + + // There are three more labels in the positive direction, + // increasing by 2π each time. + for (let i = 1; i < 4; i++) { + expect(yAxisLabels[i]).toHaveTextContent(`${2 + 2 * i}π`); + } + + // First label in the negative direction is empty + // to make room for the x axis labels (in place of -2π). + expect(yAxisLabels[4]).toHaveTextContent(""); + + // There are three more labels in the negative direction, + // decreasing by 2π each time. + for (let i = 1; i < 4; i++) { + expect(yAxisLabels[i + 4]).toHaveTextContent(`-${2 + 2 * i}π`); + } + }); + }); }); diff --git a/packages/perseus/src/widgets/interactive-graphs/interactive-graph.testdata.ts b/packages/perseus/src/widgets/interactive-graphs/interactive-graph.testdata.ts index 1ffc49f6a5..32adc957c6 100644 --- a/packages/perseus/src/widgets/interactive-graphs/interactive-graph.testdata.ts +++ b/packages/perseus/src/widgets/interactive-graphs/interactive-graph.testdata.ts @@ -459,10 +459,14 @@ export const quadraticWithStartingCoordsQuestion: PerseusRenderer = export const sinusoidWithStartingCoordsQuestion: PerseusRenderer = interactiveGraphQuestionBuilder() + .withXRange(-6 * Math.PI, 6 * Math.PI) + .withTickStep(Math.PI, 1) + .withGridStep(Math.PI, 1) + .withSnapStep(Math.PI / 2, 1) .withSinusoid({ startCoords: [ [0, 0], - [1, -1], + [3 * Math.PI, -3], ], }) .build(); @@ -1064,3 +1068,13 @@ export const graphWithLabeledFunction: PerseusRenderer = labels: [{text: "F"}], }) .build(); + +export const sinusoidWithPiTicks: PerseusRenderer = + interactiveGraphQuestionBuilder() + .withXRange(-6 * Math.PI, 6 * Math.PI) + .withYRange(-10 * Math.PI, 10 * Math.PI) + .withTickStep(Math.PI, 2 * Math.PI) + .withGridStep(Math.PI, 2 * Math.PI) + .withSnapStep(Math.PI / 2, Math.PI) + .withSinusoid() + .build();