Conversation
…ractive Graph: Fix Asymptote Drag Handle Blocked by Curve Line for Logarithm and Exponential
…ix Asymptote Drag Handle Blocked by Curve Line for Logarithm and Exponential
🗄️ Schema Change: No Changes ✅ |
|
Size Change: -2 B (0%) Total Size: 499 kB 📦 View Changed
ℹ️ View Unchanged
|
🛠️ Item Splitting: No Changes ✅ |
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (8d939a6) and published it to npm. You Example: pnpm add @khanacademy/perseus@PR3489If you are working in Khan Academy's frontend, you can run the below command. ./dev/tools/bump_perseus_version.ts -t PR3489If you are working in Khan Academy's webapp, you can run the below command. ./dev/tools/bump_perseus_version.js -t PR3489 |
There was a problem hiding this comment.
After doing 2 bug fixes I realized that I keep repeating myself giving Claude the context of what a logarithm is, so I decided to update the notes to record the expected behavior of the logarithm graph so it can be used as the context moving forward. Since we were done with the implementation having the technical research didn't make sense anymore, but still had the link to the revision for traceability.
Let me know if you have any thoughts about this because previously we discussed that we will eventually delete this file and move all decision in confluence page.
| }, | ||
| }; | ||
|
|
||
| /* Asymptote drag handle regression tests */ |
There was a problem hiding this comment.
remove these, instead created a different visual regression stories specific for the drag handle states in packages/perseus/src/widgets/interactive-graphs/__docs__/interactive-graph-asymptote-regression.stories.tsx
|
@claude review once |
There was a problem hiding this comment.
LGTM — the SVG element reorder is correct and clearly fixes the drag handle z-ordering issue.
Extended reasoning...
Overview
This PR fixes a rendering order bug in and where (including the drag handle) was rendered before (the curve) in the JSX, causing the curve's SVG path to visually occlude the drag handle. The fix swaps these two elements so the curve renders first (bottom layer) and the drag handle renders on top. Additionally, two old asymptote-focused stories are removed from the general regression file and replaced by a new dedicated stories file with 8 stories covering all drag handle states.
Security risks
None. This change only affects SVG rendering order in UI components and adds/modifies Storybook stories and documentation. There are no auth, data handling, or permission changes.
Level of scrutiny
Low. The production code change is a one-line JSX reorder in each of two files — there is no logic change, no new behavior, and no new API surface. SVG stacking order is determined purely by DOM order, so placing Plot.OfX before MovableAsymptote is exactly the correct fix.
Other factors
A bug was flagged in the new regression stories: the coordinates for the Default and NoOverlap stories appear to be inverted, meaning the stories labeled as demonstrating curve-handle overlap don't actually have overlap, and vice versa. This is a nit in the visual regression test coverage — the core fix is unaffected, and the focused-state stories (which use play functions) would still catch a reversion of the SVG order change. The mislabeled stories reduce confidence in the inactive-handle visual regression baseline but do not affect production behavior.
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
catandthemachines
left a comment
There was a problem hiding this comment.
Looks good to me. Thanks Ivy!
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @khanacademy/perseus-editor@30.3.0 ### Minor Changes - [#3474](#3474) [`b78e2ce5c3`](b78e2ce) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Add typed preview message protocol with message types, validators, and tests ### Patch Changes - Updated dependencies \[[`2f2724d244`](2f2724d), [`27e66ce3a6`](27e66ce), [`b8ed10349d`](b8ed103)]: - @khanacademy/perseus-core@24.1.1 - @khanacademy/perseus@77.2.2 - @khanacademy/keypad-context@3.2.43 - @khanacademy/kmath@2.4.1 - @khanacademy/math-input@26.4.14 - @khanacademy/perseus-linter@4.9.4 - @khanacademy/perseus-score@8.6.1 ## @khanacademy/keypad-context@3.2.43 ### Patch Changes - Updated dependencies \[[`2f2724d244`](2f2724d)]: - @khanacademy/perseus-core@24.1.1 ## @khanacademy/kmath@2.4.1 ### Patch Changes - Updated dependencies \[[`2f2724d244`](2f2724d)]: - @khanacademy/perseus-core@24.1.1 ## @khanacademy/math-input@26.4.14 ### Patch Changes - Updated dependencies \[[`2f2724d244`](2f2724d)]: - @khanacademy/perseus-core@24.1.1 - @khanacademy/keypad-context@3.2.43 ## @khanacademy/perseus@77.2.2 ### Patch Changes - [#3491](#3491) [`27e66ce3a6`](27e66ce) Thanks [@ivyolamit](https://github.com/ivyolamit)! - Interactive Graph: Fix Asymptote Line Thickness on Keyboard Focus for exponential and logarithm - [#3489](#3489) [`b8ed10349d`](b8ed103) Thanks [@ivyolamit](https://github.com/ivyolamit)! - Interactive Graph: Fix Asymptote Drag Handle Blocked by Curve Line for Logarithm and Exponential - Updated dependencies \[[`2f2724d244`](2f2724d)]: - @khanacademy/perseus-core@24.1.1 - @khanacademy/keypad-context@3.2.43 - @khanacademy/kmath@2.4.1 - @khanacademy/math-input@26.4.14 - @khanacademy/perseus-linter@4.9.4 - @khanacademy/perseus-score@8.6.1 ## @khanacademy/perseus-core@24.1.1 ### Patch Changes - [#3480](#3480) [`2f2724d244`](2f2724d) Thanks [@benchristel](https://github.com/benchristel)! - Hide exports that aren't used in frontend from the documentation. ## @khanacademy/perseus-linter@4.9.4 ### Patch Changes - Updated dependencies \[[`2f2724d244`](2f2724d)]: - @khanacademy/perseus-core@24.1.1 - @khanacademy/kmath@2.4.1 ## @khanacademy/perseus-score@8.6.1 ### Patch Changes - Updated dependencies \[[`2f2724d244`](2f2724d)]: - @khanacademy/perseus-core@24.1.1 - @khanacademy/kmath@2.4.1
Summary:
Details
The asymptote drag handle was visually hidden behind the curve line (
Plot.OfX), making it appearunclickable when the curve crossed the drag handle area. This was a rendering order issue — in SVG,
later elements in the DOM render on top of earlier ones.
Root cause: Both
logarithm.tsxandexponential.tsxrendered elements in this order:<MovableAsymptote>(includes drag handle)<Plot.OfX>(the curve) — rendered on top, blocking the drag handle<MovablePoint>componentsFix: Swap the order of
Plot.OfXandMovableAsymptoteso the curve renders first (behind),and the asymptote + drag handle renders on top of it. The final SVG layering is now:
<Plot.OfX>(curve — bottom layer)<MovableAsymptote>(asymptote line + drag handle — above the curve)<MovablePoint>components (points — top layer)Files changed:
packages/perseus/src/widgets/interactive-graphs/graphs/logarithm.tsx— moved<Plot.OfX>before<MovableAsymptote>in the JSXpackages/perseus/src/widgets/interactive-graphs/graphs/exponential.tsx— same reorderRegression stories added:
packages/perseus/src/widgets/interactive-graphs/__docs__/interactive-graph-asymptote-regression.stories.tsx*DragHandleDefault— inactive handle visible above the curve (curve overlaps handle area)*DragHandleFocused— focus ring + active pill with grip dots above the curve (viaplayfunction)*PointFocusedHandleInactive— point has focus ring, handle stays small/inactive (viaplayfunction)*DragHandleNoOverlap— baseline with asymptote away from curve for comparisonplayfunctions to programmatically focus elements so states are visible in both Storybook UIand Chromatic snapshots
tags: ["!autodocs"]to show only individual stories without a Docs page (matches radiointeractions pattern)
Technical notes updated:
packages/perseus/src/widgets/interactive-graphs/__docs__/notes/logarithm.md— rewrote fromresearch/POC format to a technical reference documenting all expected behavior, architecture,
decisions, and implementation notes. Added SVG rendering order and drag handle focus behavior
as documented expected behavior. Intended as context for future Claude Code sessions.
Co-Authored by Claude Code (Opus)
Issue: LEMS-4037
Test plan: