[Interactive Graph] Fix interactive graph x-axis label overlapping with content below#3567
[Interactive Graph] Fix interactive graph x-axis label overlapping with content below#3567
Conversation
…active graph x-axis label overlapping with content below when y-range starts at 0 or higher
…teractive graph x-axis label overlapping with content below
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (3058c6e) and published it to npm. You Example: pnpm add @khanacademy/perseus@PR3567If you are working in Khan Academy's frontend, you can run the below command. ./dev/tools/bump_perseus_version.ts -t PR3567If you are working in Khan Academy's webapp, you can run the below command. ./dev/tools/bump_perseus_version.js -t PR3567 |
🗄️ Schema Change: No Changes ✅ |
🛠️ Item Splitting: No Changes ✅ |
|
Size Change: +142 B (+0.03%) Total Size: 503 kB 📦 View Changed
ℹ️ View Unchanged
|
|
@claude review once |
There was a problem hiding this comment.
Logic and tests look solid, but this is a visual fix and the test plan still has unchecked Storybook verification items across multiple y-range configurations — worth a human eye on the actual rendering before merging.
Extended reasoning...
Overview
This PR fixes an x-axis label overflow issue in the interactive-graph widget. It introduces a new getGraphBottomMargin() helper in backgrounds/utils.ts that replaces the hardcoded marginBottom: "30px" in mafs-graph.tsx with a value computed from the label position, graph height, and whether axis labels are shown. Seven unit tests cover the new helper, and nine snapshot files update purely because a styled-component CSS hash changes when the inline style becomes dynamic.
Security risks
None. This is a CSS layout calculation in a rendering widget — no auth, crypto, data handling, or user-input parsing involved.
Level of scrutiny
Moderate. The function is small, pure, and has good unit-test coverage covering normal range, yMin=0, wholly-positive ranges, alongEdge labels, and the disabled cases (no label / markings off). The math matches the existing clampLabelPosition and getLabelPosition behaviour. However, this is a visual fix shipping to a heavily used widget, and the PR's own test plan lists six unchecked Storybook verification items spanning different y-range configurations and label modes — those are exactly the cases where a margin regression would be felt by content authors.
Other factors
- Author explicitly requested review (
@claude review once). - Bug-hunting system found no issues.
- Snapshot churn is mechanical (CSS class hash), not behavioral.
- The default
[-10, 10]case still resolves to the original 30px margin, so no behavior change there — but I'd want a human to confirm visual parity in the unchecked Storybook scenarios before approving.
Summary:
[0, 19]), the x-axis label extends below the graph content area and overlaps with text rendered after the widget.marginBottom: 30pxthat didn't account for label overflow.Root Cause
The x-axis label is absolutely positioned at the end of the x-axis. When
yMin >= 0, the x-axis sits at or near the bottom edge of the graph, pushing the label below the graph's content box. The fixed 30px margin was not enough to cover the overflow, especially for:height + fontSize * 1.25)yMin >= 0(label offset byfontSize * 3below the graph)Changes
packages/perseus/src/widgets/interactive-graphs/backgrounds/utils.tsgetGraphBottomMargin()— calculates the bottom margin needed to clear the x-axis label overflow so it doesn't overlap with adjacent content.packages/perseus/src/widgets/interactive-graphs/mafs-graph.tsxmarginBottom: "30px"with a dynamic value fromgetGraphBottomMargin()packages/perseus/src/widgets/interactive-graphs/backgrounds/axis-labels.test.tsgetGraphBottomMargin()covering: normal range, yMin=0 (onAxis), wholly positive range (onAxis clamped), alongEdge with yMin=0, no label text, labels hidden, and negative rangepackages/perseus/src/widgets/interactive-graphs/__snapshots__/interactive-graph.test.tsx.snapEffective margins by scenario
Co-authored by Claude Code (Opus)
Issue: LEMS-3921
Test plan
Use the content below:
[0, 19]and x-label$\text{Time (hours)}$[-1, 19](workaround case) still looks correct[5, 15]alongEdgelabel location[-10, 10]is unchanged