[Interactive Graph] Wire pointLabels through linear-system and segment graphs for custom points label#3673
Conversation
|
Size Change: 0 B Total Size: 506 kB ℹ️ View Unchanged
|
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (8cb20b7) and published it to npm. You Example: pnpm add @khanacademy/perseus@PR3673If you are working in Khan Academy's frontend, you can run the below command. ./dev/tools/bump_perseus_version.ts -t PR3673If you are working in Khan Academy's webapp, you can run the below command. ./dev/tools/bump_perseus_version.js -t PR3673 |
| @@ -35,48 +42,39 @@ const StartCoordsMultiline = (props: Props) => { | |||
| } | |||
| expanded={true} | |||
| > | |||
There was a problem hiding this comment.
Swapped CoordinatePairInput → CoordInput here so multi-line picks up the same per-endpoint Label [_] field the other start-coords editors got in PRs 2–4. CoordInput is the shared wrapper that adds the heading + optional label TextField on top of CoordinatePairInput; the lower-level component is still used (just one layer up). Also drops the local Aphrodite / Strut / spacing imports since CoordInput handles layout.
| pointLabels?: ReadonlyArray<string>; | ||
| onChangePointLabels?: (pointLabels: ReadonlyArray<string>) => void; |
There was a problem hiding this comment.
Same question as the last PR. StartCoordsMultiline is only called one place and you're passing these to it; why make these optional?
| onPointLabelChange={ | ||
| onChangePointLabels && | ||
| ((newLabel) => updatePointLabel(i * 2, newLabel)) | ||
| } |
There was a problem hiding this comment.
If onChangePointLabels was required, this would be simpler.
| }, | ||
| ); | ||
|
|
||
| it("renders point name fields when onChangePointLabels is provided", () => { |
There was a problem hiding this comment.
If onChangePointLabels was required, you wouldn't need these two tests.
| onChange={() => {}} | ||
| onChangePointLabels={onChangePointLabelsMock} | ||
| />, | ||
| {wrapper: RenderStateRoot}, |
There was a problem hiding this comment.
I wonder if there's a Jest/RTL setting we can set to auto-wrap things for us.
| const seg2Point1NameInput = screen.getAllByRole("textbox", { | ||
| name: "Point 1 name", | ||
| })[1]; |
There was a problem hiding this comment.
This to me is an a11y red flag (for the content creation UI): you have two textboxes in the same component with the same name.
It might be too big of a lift to fix right now, just calling it out. The reason I noticed it was because the test was confusing to me ("why is the third element in toHaveBeenLastCalledWith getting changed?"..."Oh, because it's the second segment."), so I would recommend adding some comments.
| const buildLabel = (index: number, point: vec.Vector2) => | ||
| buildPointAriaLabel(pointLabels, index, point, strings, locale); |
| point1AriaLabel: | ||
| buildLabel(i * 2, line[0]) ?? | ||
| strings.srLinearSystemPoint({ | ||
| lineNumber: i + 1, | ||
| pointSequence: 1, | ||
| x: srFormatNumber(line[0][0], locale), | ||
| y: srFormatNumber(line[0][1], locale), | ||
| }), |
There was a problem hiding this comment.
nit: Still would be nice if this was pulled out of JSX and into the logic section of the component.
| const interactiveElements = screen.getAllByRole("button"); | ||
| const [point1, , point2] = interactiveElements; | ||
|
|
||
| // Assert | ||
| expect(point1).toHaveAttribute("aria-label", "Point A at -5 comma 5."); | ||
| expect(point2).toHaveAttribute("aria-label", "Point B at 5 comma 5."); |
| const buildLabel = (index: number, point: vec.Vector2) => | ||
| buildPointAriaLabel(pointLabels, index, point, strings, locale); |
There was a problem hiding this comment.
This is so close to just having a custom hook, which could abstract away buildPointAriaLabel's API, the use of usePerseusI18n, and could be shared between components.
| gridStep: [1, 1], | ||
| snapStep: [1, 1], | ||
| step: [1, 1], | ||
| range: [ | ||
| [-10, 10], | ||
| [-10, 10], | ||
| ], |
There was a problem hiding this comment.
Same here, I think generateInteractiveGraphQuestion will default a lot of this stuff for you.
catandthemachines
left a comment
There was a problem hiding this comment.
Looks good, had one question on this test. 😉
| ); | ||
| }); | ||
|
|
||
| it("falls back to the per-line default for truthy non-string entries", () => { |
There was a problem hiding this comment.
Interesting safe guard. Do you foresee something like this happening in the current implementation, or just safeguarding against the potential for this to occur?
There was a problem hiding this comment.
This pattern of test was added in one of your comment in previous PR. this scenario is an edge case when the input is coming from the json object when creating a widget.
## Summary: - Adds the foundation that PRs 3–7 will reuse: state plumbing on `InteractiveGraphStateCommon`, a `buildPointAriaLabel` helper, an optional `pointLabel` prop bundle on `CoordInput`, and a per-type-aware `changePointLabels` editor handler that already covers all 15 graph types so follow-up PRs don't have to touch it. - Wires it through the **point** graph end-to-end. After this PR, the LEMS-3995 reference question announces *"Point T at zero comma zero"* instead of *"Point 1 at zero comma zero"*. - Existing content is unchanged: when `pointLabels` is unset, the announcement falls back to the original numeric `"Point 1"`, `"Point 2"`, … default. - The author-facing tip that originally sat under the "Start coordinates" heading was dropped before merge — only `point` graphs expose the name field in PR 2 (polygon shares `start-coords-point.tsx` but is gated off in `start-coords-settings.tsx` until PR 3 wires its render side), so a global tip would mislead authors of every other graph type. It will return as a tooltip on the per-row name field once PRs 3–7 land (tracked in the plan doc's Follow-up work section). ## The bug Interactive coordinate-plane questions ask the learner to plot a specific named point (e.g. *"Plot point T to complete the rectangle"*), but JAWS announces it as *"Point 1 at zero comma zero"*. The label is hard-coded to a numeric index in `useControlPoint`, so it never matches the prompt. [PR 1](#3605) added the optional `pointLabels` field to every interactive graph-type schema. The field's shape mirrors `coords` / `startCoords` per type: `[string, string, string]` for `angle` / `quadratic`, `[string, string]` for `linear` / `ray` / `vector` / `absolute-value`, and `string[]` for the variable-arity rest. This PR makes that field actually do something — for `point` graphs first. Co-authored by Claude Code (Opus) Issue: LEMS-3995 ## Test plan - [x] `pnpm typecheck` — passes - [x] `pnpm lint packages/perseus packages/perseus-core packages/perseus-editor` — passes - [x] `pnpm prettier . --check` — passes - [x] `pnpm jest packages/perseus/src/widgets/interactive-graphs packages/perseus-editor/src/widgets/interactive-graph-editor packages/perseus-editor/src/widgets/__tests__/interactive-graph-editor.test.tsx` — 1584 tests pass (28 pre-existing skips) - [ ] Reviewer manual: open the `PointWithCustomLabel` story, enable Storybook a11y addon (or VoiceOver/JAWS), and confirm the announcement matches *"Point T at 0 comma 0"*. - [ ] Reviewer manual: open the `PointWithDefaultLabel` story, enable Storybook a11y addon (or VoiceOver/JAWS), and confirm the announcement matches *"Point 1 at 0 comma 0"*. Confirm default stories without `pointLabels` are unchanged. ## PR Series and Follow-ups (PRs 3–7) Independent and parallelizable. None of them touch `data-schema.ts`, the parser, `strings.ts`, `InteractiveGraphStateCommon`, `interactive-graph-editor.tsx`'s handler/serialization, or `buildPointAriaLabel`. They do widen the `type === "point"` gate in `start-coords-settings.tsx` to include each new graph type as it lights up. - **PR 1** — [Schema — no behavior. Only data-shape stop.](#3605) - 👉🏼 **PR 2** — [Foundation + Point graph](#3643) - **PR 3** — [Polygon (shares start-coords-point.tsx); widen the gate to type === "point" || type === "polygon"](#3669) - **PR 4** — [Lines: linear, ray](#3672) - **PR 5** — [Multi-line: linear-system, segment](#3673) - **PR 6** — Curves: quadratic, sinusoid, tangent, exponential, logarithm. - **PR 7** — Special shapes: angle, circle, absolute-value. Author: ivyolamit Reviewers: catandthemachines, ivyolamit, handeyeco Required Reviewers: Approved By: handeyeco, catandthemachines Checks: ✅ 12 checks were successful, ⏭️ 1 check has been skipped Pull Request URL: #3643
…m points label (#3669) ## Summary: - Wires `pointLabels` through polygon graphs (Limited, Unlimited closed, and Unlimited open) end-to-end on the render side and unlocks the editor label fields that were already plumbed in PR 2. - Each `MovablePoint` on a polygon now announces its author-supplied label (e.g. *"Point A at -2 comma -1"*); the polygon's interactive-elements summary uses the same labels. Indices without a custom label fall back to the existing numeric *"Point N at …"* default per index. Co-authored by Claude Code (Opus) Issue: LEMS-3995 ## Risk Low. No foundation changes. The new code paths are: - Two `MovablePoint` call sites (one per polygon variant) gaining an `ariaLabel` prop that falls back to `undefined` when no custom label is set. - One per-point string-builder branch in `describePolygonGraph`, now routed through `buildPointAriaLabel` so it shares fallback rules with the focusable handle. - One gate removal in the editor. Reviewer focus: confirm the SR-tree summary, the polygon's overall `aria-label`, and each `MovablePoint`'s accessible name all use the same labels when `pointLabels` is set, and all fall back consistently when it isn't (including the truthy-non-string defense-in-depth case the new test locks in). ## Test plan - [x] `pnpm tsc` — passes - [x] `pnpm lint packages/perseus packages/perseus-editor` — passes - [x] `pnpm prettier . --check` — passes - [x] `pnpm test packages/perseus/src/widgets/interactive-graphs` — 1114 pass, 28 pre-existing skips, 12 snapshots - [x] `pnpm --filter perseus-editor test` — passes - [ ] Reviewer manual: open the `PolygonWithCustomLabels` story, enable the Storybook a11y addon (or VoiceOver / JAWS), and confirm each vertex announces *"Point A / B / C at …"* matching the prompt. Confirm the existing `Polygon` story (no `pointLabels`) still announces the legacy *"Point 1 / 2 / 3 at …"* defaults. - [ ] Reviewer manual: open the `InteractiveGraphPolygon` editor story, expand "Start coordinates", type a letter into one of the polygon vertices' label fields, and confirm the rendered graph's vertex `aria-label` *and* the **Screen reader tree** panel both update on the same render — same dual-write contract PR 2 added for `point`. ## PR Series and Follow-ups (PRs 4–7) Independent and parallelizable. None of them touch the foundation files (`data-schema.ts`, the parser, `strings.ts`, `InteractiveGraphStateCommon`, `interactive-graph-editor.tsx`'s handler/serialization, or `buildPointAriaLabel` / `CoordInput`). Each one wires its graph type's render side and adds the per-type `start-coords-<type>.tsx` plumbing using the same `CoordInput` `pointLabel` prop bundle PR 2 introduced. - **PR 1** — [Schema — no behavior. Only data-shape stop.](#3605) - **PR 2** — [Foundation + Point graph](#3643) - 👉🏼 **PR 3** — [Polygon (shares start-coords-point.tsx); widen the gate to type === "point" || type === "polygon"](#3669) - **PR 4** — [Lines: linear, ray](#3672) - **PR 5** — [Multi-line: linear-system, segment](#3673) - **PR 6** — Curves: quadratic, sinusoid, tangent, exponential, logarithm. - **PR 7** — Special shapes: angle, circle, absolute-value. Author: ivyolamit Reviewers: SonicScrewdriver, catandthemachines, handeyeco Required Reviewers: Approved By: SonicScrewdriver, catandthemachines Checks: ✅ 12 checks were successful, ⏭️ 1 check has been skipped Pull Request URL: #3669
8fee869 to
8dd0a78
Compare
…custom points label (#3672) ## Summary: - Wires `pointLabels` through two of the three line-family interactive graphs — `linear` and `ray` — end-to-end on the render side and unlocks the editor label fields for those two types. - Each interactive endpoint now announces its author-supplied label (e.g. *"Point A at -5 comma 5"*); when no custom label is set the existing default behavior is preserved per graph type: - **linear** falls back to the generic *"Point 1 at …"* / *"Point 2 at …"* defaults (same as before). - **ray** falls back to the semantic *"Endpoint at …"* / *"Through point at …"* defaults (same as before). - **Vector is excluded** from this PR and from the LEMS-3995 rollout entirely. The `pointLabels` field on `PerseusGraphTypeVector` (originally shipped by PR 1) is **removed from the schema and parser in this PR** so the type reflects the runtime behaviour. The original plan listed `vector` alongside `linear` and `ray` because all three share the `[string, string]` schema shape and the `start-coords-line.tsx` editor component. The vector has tail and tip which is already mathematically correct for screen readers. Issue: LEMS-3995 Co-Authored by Claude Code (Opus) ## Risk Low. The new code paths are: - Two render files (linear, ray) each gaining a few lines that read `pointLabels` from state and pass the helper output as `ariaLabel`. - `start-coords-line.tsx` extended with `pointLabels` / `onChangePointLabels` props, the flat `pointLabel` / `onPointLabelChange` forwarded to each `<CoordInput>`, and a small `updatePointLabel` helper that pads the 2-tuple shape. Vector continues to call `<StartCoordsLine>` without those props, so its editor UX is unchanged. - One gate widening per case branch in `start-coords-settings.tsx` (only linear / ray; vector deliberately not widened). - The schema cleanup (removing `pointLabels` from `PerseusGraphTypeVector` and the parser) is two line deletions touching `data-schema.ts` and `interactive-graph-widget.ts`. Because the field was only ever optional and was never written to by the editor or read at runtime, no production content can break: any content that *does* carry `pointLabels` on a vector was hand-authored (the editor never produced it) and the field was already a no-op. - One case relocation in `reshape-point-labels.ts` (`case "vector":` moves from `toPair` to the no-op group) — required for TypeScript to keep type-checking after the schema field disappears. Reviewer focus: - For each of linear / ray, (a) the focusable handle uses the custom label when set, (b) the right default is preserved when it's not (generic vs semantic). - For vector, confirm that nothing changed at runtime: the SR announcements are still `"Tip point at X comma Y"` regardless of any `pointLabels` value (now impossible to set via the schema), and the editor's `<StartCoordsLine>` for vector still renders without the `Label [_]` field. - **Schema cleanup verification.** Confirm no consumer of `PerseusGraphTypeVector` reads `pointLabels`. The `pnpm tsc` pass is the structural lock — any consumer that *was* reading it would fail compilation now. The full `perseus-core` jest suite is the runtime lock — any parser regression test asserting the field was accepted on vector would fail; none did, which means no such test existed (the field was never authored in fixtures). - The rationale for excluding vector — see [Vector intentionally skipped](#vector-intentionally-skipped). Anyone uncomfortable with the schema-level removal can be directed at the decision log entry in the plan doc. ## Test plan - [x] `pnpm tsc` — passes (the schema cleanup is structurally consistent across `data-schema.ts`, `interactive-graph-widget.ts`, and `reshape-point-labels.ts`) - [x] `pnpm lint <changed files>` — passes - [x] `pnpm prettier --check <changed files>` — passes - [x] `pnpm jest packages/perseus-core packages/perseus-editor/src/widgets/interactive-graph-editor/utils/reshape-point-labels.test.ts packages/perseus/src/widgets/interactive-graphs/graphs/{linear,ray,vector}.test.tsx packages/perseus-editor/src/widgets/interactive-graph-editor/start-coords/start-coords-settings.test.tsx` — 1762/1763 pass, 1 pre-existing skip. Includes the full `perseus-core` suite (parser regression tests, generator tests) as a non-regression check for the schema cleanup; includes `reshape-point-labels.test.ts` to confirm the relocated vector case behaves the same as `none`. - [ ] Reviewer manual: open the `LinearWithCustomLabels` / `RayWithCustomLabels` stories. Enable the Storybook a11y addon (or VoiceOver / JAWS) and confirm each interactive endpoint announces *"Point A at …"* / *"Point B at …"* matching its prompt. Confirm the existing `Linear` / `Ray` stories still announce the legacy defaults. - [ ] Reviewer manual: open the `InteractiveGraphLinear` / `InteractiveGraphRay` editor stories, expand "Start coordinates", and confirm each row reads as `Point N:`. ## PR Series and Follow-ups (PRs 5–7) Independent and parallelizable. None of them touch the foundation files. Each wires its graph type's render side and adds the per-type `start-coords-<type>.tsx` plumbing using the same flat `CoordInput` `pointLabel` / `onPointLabelChange` props PR 2 take-2 introduced. - **PR 1** — [Schema — no behavior. Only data-shape stop.](#3605) - **PR 2** — [Foundation + Point graph](#3643) - **PR 3** — [Polygon (shares start-coords-point.tsx); widen the gate to type === "point" || type === "polygon"](#3643) - 👉🏼 **PR 4** — [Lines: linear, ray](#3672) - **PR 5** — [Multi-line: linear-system, segment](#3673) - **PR 6** — Curves: quadratic, sinusoid, tangent, exponential, logarithm. - **PR 7** — Special shapes: angle, circle, absolute-value. Author: ivyolamit Reviewers: ivyolamit, handeyeco, catandthemachines Required Reviewers: Approved By: catandthemachines, handeyeco Checks: ✅ 12 checks were successful, ⏭️ 1 check has been skipped Pull Request URL: #3672
…Interactive Graph] Wire pointLabels through linear-system and segment graphs for custom points label
…] Wire pointLabels through linear-system and segment graphs for custom points label
33de3b3 to
dfc0580
Compare
… address code review comments
Summary:
pointLabelsthrough the two multi-line interactive graphs —linear-systemandsegment— end-to-end on the render side and unlocks the editor label fields for both types.srLinearSystemPointtemplate).srSingleSegmentGraphEndpointAriaLabel/srMultipleSegmentGraphEndpointAriaLabeltemplates, depending on segment count).pointLabelsfor both types is a single flat array across all lines/segments (string[]schema):[line0.p1, line0.p2, line1.p1, line1.p2, …]. Renderer maps(lineIndex i, endpointIndex j) → flat index i * 2 + j. The editor'supdatePointLabel(flatIndex, newLabel)helper rebuilds the fullstartCoords.length * 2-length array on every keystroke so the unedited slots stay as their existing value (or"").Co-Authored by Claude Code (Opus)
Issue: LEMS-3995
Risk
Low. No foundation changes. The new code paths are:
buildLabelclosure and a?? defaultLabelchain on eachMovableLine.ariaLabelsslot.start-coords-multiline.tsxrefactored to consumeCoordInput(the shared two-row tile component) withpointLabels/onChangePointLabelsplumbed through, plus a localupdatePointLabelhelper that pads the flat array.start-coords-settings.tsx(linear-system and segment, mirroring how PR 2/3/4 widened thepoint/polygon/linear/raycases).Reviewer focus:
i * 2 + jis consistent across the renderer (linear-system.tsx,segment.tsx) and the editor (start-coords-multiline.tsx'supdatePointLabel). The render-side test cases assert both endpoints of both lines/segments to lock the indexing in.buildLabel(...) ?? formatSegment(...)correctly preserves segment's single-vs-multi default phrasing (Endpoint N at …vsEndpoint N on segment M at …) — locked in by separate tests for single-segment and multi-segment states.start-coords-multiline.tsxrefactor dropsStrut/spacing/ Aphrodite — these were the only remaining users of those imports in this file, so the removal is clean.Test plan:
pnpm tsc— passespnpm lint <changed files>— passespnpm prettier --check <changed files>— passespnpm jest packages/perseus/src/widgets/interactive-graphs/graphs/linear-system.test.tsx packages/perseus/src/widgets/interactive-graphs/graphs/segment.test.tsx packages/perseus-editor/src/widgets/interactive-graph-editor/start-coords/start-coords-settings.test.tsx— 173 pass, 0 failLinearSystemWithCustomLabels/SegmentWithCustomLabelsstories. Enable the Storybook a11y addon (or VoiceOver / JAWS) and confirm each interactive endpoint announces "Point A / B / C / D at …" matching its prompt. Confirm the existingLinearSystem/Segmentstories still announce the legacy per-line / per-segment defaults.InteractiveGraphLinearSystem/InteractiveGraphSegmenteditor stories, expand "Start coordinates", and confirm each line/segment accordion now shows the sharedCoordInputstacked tile (Point N:heading on top,x [_] y [_]coord row,Label [_]field below) instead of the previous bespoke single-rowPoint 1: x [_] y [_]layout. Type a letter into one of the label fields and confirm the rendered graph's endpointaria-labeland the Screen reader tree panel update on the same render.PR Series and Follow-ups (PRs 6–7)
Independent and parallelizable. None of them touch the foundation files. Each wires its graph type's render side and adds the per-type
start-coords-<type>.tsxplumbing using the same flatCoordInputpointLabel/onPointLabelChangeprops PR 2 take-2 introduced.