[Interactive Graph] Wire pointLabels through line and ray graphs for custom points label#3672
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 (8dd0a78) and published it to npm. You Example: pnpm add @khanacademy/perseus@PR3672If you are working in Khan Academy's frontend, you can run the below command. ./dev/tools/bump_perseus_version.ts -t PR3672If you are working in Khan Academy's webapp, you can run the below command. ./dev/tools/bump_perseus_version.js -t PR3672 |
There was a problem hiding this comment.
We're cleaning up the data type related to vector pointLabels since we are excluding it that was introduced here. The vector screen reader is mathematically correct already having the tail and tip terms.
There was a problem hiding this comment.
I'm concerned about the change to vector in this PR, but if you tell me that's fine I can approve.
| label="Point 2" | ||
| coord={startCoords[1]} | ||
| onChange={(value) => onChange([startCoords[0], value])} | ||
| pointLabel={pointLabels?.[1]} |
There was a problem hiding this comment.
(Random thought, not a blocker) These two uses of CoordInput seem so similar, I wonder if they could just be called in a loop. The benefit would be that they're less likely to get out of sync. For example you had to add onPointLabelChange twice when making this change; if we consolidated them into one loop, you'd only have to add onPointLabelChange in one spot.
| onChange={() => {}} | ||
| onChangePointLabels={() => {}} | ||
| />, | ||
| {wrapper: RenderStateRoot}, |
There was a problem hiding this comment.
Just a question: do you actually need RenderStateRoot here? If not, should we remove it? If so, should we make an abstraction around this?
There was a problem hiding this comment.
Yes, RenderStateRoot is needed for tests that render the label TextField.
| state={{ | ||
| ...baseLinearState, | ||
| // eslint-disable-next-line no-restricted-syntax -- cast simulates malformed JSON the parser would reject | ||
| pointLabels: [42, "B"] as unknown as string[], |
There was a problem hiding this comment.
This is when content editor use the json object to create the content/widget/
| const overallGraphLabel = "A ray on a coordinate plane."; | ||
|
|
||
| describe("Linear graph screen reader", () => { | ||
| describe("Ray graph screen reader", () => { |
| point1AriaLabel: buildPointAriaLabel( | ||
| pointLabels, | ||
| 0, | ||
| line[0], | ||
| strings, | ||
| locale, | ||
| ), |
There was a problem hiding this comment.
buildPointAriaLabels API still feels a little awkward to me. I think I brought this up in another PR and I'm guessing you determined it was best as-is. No action needed, just wanted to point it out again.
There was a problem hiding this comment.
I just saw Cat's PR, i'll revisit this again in this ticket: see note in the ticket.
https://khanacademy.atlassian.net/browse/LEMS-4206
| point1AriaLabel: | ||
| buildPointAriaLabel( | ||
| pointLabels, | ||
| 0, | ||
| line[0], | ||
| strings, | ||
| locale, | ||
| ) ?? srRayEndpoint, | ||
| point2AriaLabel: | ||
| buildPointAriaLabel( | ||
| pointLabels, | ||
| 1, | ||
| line[1], | ||
| strings, | ||
| locale, | ||
| ) ?? srRayTerminalPoint, |
There was a problem hiding this comment.
I wonder if it would be worth moving these out of JSX:
const point1AriaLabel = buildPointAriaLabel(
pointLabels,
0,
line[0],
strings,
locale,
) ?? srRayEndpointThen:
| point1AriaLabel: | |
| buildPointAriaLabel( | |
| pointLabels, | |
| 0, | |
| line[0], | |
| strings, | |
| locale, | |
| ) ?? srRayEndpoint, | |
| point2AriaLabel: | |
| buildPointAriaLabel( | |
| pointLabels, | |
| 1, | |
| line[1], | |
| strings, | |
| locale, | |
| ) ?? srRayTerminalPoint, | |
| point1AriaLabel, | |
| point2AriaLabel, |
| gridStep: [1, 1], | ||
| snapStep: [1, 1], | ||
| step: [1, 1], | ||
| range: [ | ||
| [-10, 10], | ||
| [-10, 10], | ||
| ], |
There was a problem hiding this comment.
Doesn't generateInteractiveGraphQuestion default a lot of this stuff for you?
catandthemachines
left a comment
There was a problem hiding this comment.
Looks good to me, did have one concern on the vector case in utils.ts but non blocking as long as it's correct.
## 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
22d5294 to
dd5fde6
Compare
…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
3276b6e to
4beeaa4
Compare
|
Note: The data-schema change here for vector is Ok since this is new type that was added recently and not used in prod right now. |
…Interactive Graph] Wire pointLabels through line and ray graphs for custom points label
…] Wire pointLabels through line and ray graphs for custom points label
…d address review comments
8fee869 to
8dd0a78
Compare
|
Rebased from latest master to pickup the recent change introduced in #3682 |
@handeyeco thanks for the review, regarding your concern about vector. This change is okay. See my response in the comment above here: #3672 (comment) |
…t graphs for custom points label (#3673) ## Summary: - Wires `pointLabels` through the two multi-line interactive graphs — `linear-system` and `segment` — end-to-end on the render side and unlocks the editor label fields for both 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 per-type defaults are preserved unchanged: - **linear-system** falls back to *"Point N on line M at …"* (the `srLinearSystemPoint` template). - **segment** falls back to *"Endpoint N at …"* / *"Endpoint N on segment M at …"* (the `srSingleSegmentGraphEndpointAriaLabel` / `srMultipleSegmentGraphEndpointAriaLabel` templates, depending on segment count). - `pointLabels` for 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's `updatePointLabel(flatIndex, newLabel)` helper rebuilds the full `startCoords.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: - Two render files (linear-system, segment) each gaining a per-component `buildLabel` closure and a `?? defaultLabel` chain on each `MovableLine.ariaLabels` slot. - `start-coords-multiline.tsx` refactored to consume `CoordInput` (the shared two-row tile component) with `pointLabels` / `onChangePointLabels` plumbed through, plus a local `updatePointLabel` helper that pads the flat array. - One gate widening per case branch in `start-coords-settings.tsx` (linear-system and segment, mirroring how PR 2/3/4 widened the `point` / `polygon` / `linear` / `ray` cases). Reviewer focus: - The flat-array indexing `i * 2 + j` is consistent across the renderer (`linear-system.tsx`, `segment.tsx`) and the editor (`start-coords-multiline.tsx`'s `updatePointLabel`). The render-side test cases assert both endpoints of both lines/segments to lock the indexing in. - The fallback chain `buildLabel(...) ?? formatSegment(...)` correctly preserves segment's single-vs-multi default phrasing (`Endpoint N at …` vs `Endpoint N on segment M at …`) — locked in by separate tests for single-segment and multi-segment states. - The `start-coords-multiline.tsx` refactor drops `Strut` / `spacing` / Aphrodite — these were the only remaining users of those imports in this file, so the removal is clean. ## Test plan: - [x] `pnpm tsc` — passes - [x] `pnpm lint <changed files>` — passes - [x] `pnpm prettier --check <changed files>` — passes - [x] `pnpm 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 fail - [ ] Reviewer manual: open the `LinearSystemWithCustomLabels` / `SegmentWithCustomLabels` stories. 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 existing `LinearSystem` / `Segment` stories still announce the legacy per-line / per-segment defaults. - [ ] Reviewer manual: open the `InteractiveGraphLinearSystem` / `InteractiveGraphSegment` editor stories, expand "Start coordinates", and confirm each line/segment accordion now shows the shared `CoordInput` stacked tile (`Point N:` heading on top, `x [_] y [_]` coord row, `Label [_]` field below) instead of the previous bespoke single-row `Point 1: x [_] y [_]` layout. Type a letter into one of the label fields and confirm the rendered graph's endpoint `aria-label` *and* 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>.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: handeyeco, catandthemachines Checks: ✅ 12 checks were successful, ⏭️ 1 check has been skipped Pull Request URL: #3673
Summary:
pointLabelsthrough two of the three line-family interactive graphs —linearandray— end-to-end on the render side and unlocks the editor label fields for those two types.pointLabelsfield onPerseusGraphTypeVector(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 listedvectoralongsidelinearandraybecause all three share the[string, string]schema shape and thestart-coords-line.tsxeditor 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:
pointLabelsfrom state and pass the helper output asariaLabel.start-coords-line.tsxextended withpointLabels/onChangePointLabelsprops, the flatpointLabel/onPointLabelChangeforwarded to each<CoordInput>, and a smallupdatePointLabelhelper that pads the 2-tuple shape. Vector continues to call<StartCoordsLine>without those props, so its editor UX is unchanged.start-coords-settings.tsx(only linear / ray; vector deliberately not widened).pointLabelsfromPerseusGraphTypeVectorand the parser) is two line deletions touchingdata-schema.tsandinteractive-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 carrypointLabelson a vector was hand-authored (the editor never produced it) and the field was already a no-op.reshape-point-labels.ts(case "vector":moves fromtoPairto the no-op group) — required for TypeScript to keep type-checking after the schema field disappears.Reviewer focus:
"Tip point at X comma Y"regardless of anypointLabelsvalue (now impossible to set via the schema), and the editor's<StartCoordsLine>for vector still renders without theLabel [_]field.PerseusGraphTypeVectorreadspointLabels. Thepnpm tscpass is the structural lock — any consumer that was reading it would fail compilation now. The fullperseus-corejest 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).Test plan
pnpm tsc— passes (the schema cleanup is structurally consistent acrossdata-schema.ts,interactive-graph-widget.ts, andreshape-point-labels.ts)pnpm lint <changed files>— passespnpm prettier --check <changed files>— passespnpm 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 fullperseus-coresuite (parser regression tests, generator tests) as a non-regression check for the schema cleanup; includesreshape-point-labels.test.tsto confirm the relocated vector case behaves the same asnone.LinearWithCustomLabels/RayWithCustomLabelsstories. 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 existingLinear/Raystories still announce the legacy defaults.InteractiveGraphLinear/InteractiveGraphRayeditor stories, expand "Start coordinates", and confirm each row reads asPoint 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>.tsxplumbing using the same flatCoordInputpointLabel/onPointLabelChangeprops PR 2 take-2 introduced.