[ Interactive Graph | Vector Graph ] PR3: Rendering & Accessibility#3441
[ Interactive Graph | Vector Graph ] PR3: Rendering & Accessibility#3441SonicScrewdriver wants to merge 15 commits intoLEMS-3971/vector-pr2from
Conversation
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.
🗄️ Schema Change: No Changes ✅ |
🛠️ Item Splitting: No Changes ✅ |
|
Size Change: +2.25 kB (+0.45%) Total Size: 501 kB 📦 View Changed
ℹ️ View Unchanged
|
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (ec0075a) and published it to npm. You Example: pnpm add @khanacademy/perseus@PR3441If you are working in Khan Academy's frontend, you can run the below command. ./dev/tools/bump_perseus_version.ts -t PR3441If you are working in Khan Academy's webapp, you can run the below command. ./dev/tools/bump_perseus_version.js -t PR3441 |
cd4b7ce to
68ae5c4
Compare
77f5629 to
52d299e
Compare
8e83938 to
cec842e
Compare
fbb7178 to
705a8e0
Compare
acb02ad to
3e10b58
Compare
86a5da9 to
da4b606
Compare
bd44dd3 to
0c99f50
Compare
ivyolamit
left a comment
There was a problem hiding this comment.
Adding here my initial comment for the first pass. I'm happy to chat about the focus state behavior in chat.
There was a problem hiding this comment.
Something I fixed recently based on the playtest feedback on the drag handle focus, the expected behavior is when the user drags using the pill and the mouse hover away from the pill it will retain the focus state until user navigates away or navigate to another point whether it's using keyboard or mouse.
| Vector | Logarithm |
|---|---|
![]() |
![]() |
Suggestion: retaining the pill focus to have consistent behavior. I'm happy to discuss this with you 😉
There was a problem hiding this comment.
Ooo great point, I'll match that!
There was a problem hiding this comment.
Hmmm I reviewed the play test feedback and I see a message from our designer specifically requesting that the drag handle should lose the focus state when the mouse moves out, which is how I implemented it. Should we check in with them before adjusting?
There was a problem hiding this comment.
Also a nit pick: what you think of naming this drag-handle-pill.tsx DragHandlePill?
edit: it also matches how we're naming most things by using the action first.
There was a problem hiding this comment.
There was a problem hiding this comment.
Oooo makes sense to me! I didn't think about the order of the words so much while I was renaming it to be more generic.
I've also discussed the minimum distance with design/content but I haven't heard back yet. I might suggest that we leave this "as is" so that we can get feedback during the play test about minimum distances for the vector graph. ;) However, I can also implement a 2-grid-step minimum now if you feel we should make an executive decision
ivyolamit
left a comment
There was a problem hiding this comment.
@SonicScrewdriver here's my second pass, I think overall it's looking good. Let's chat about the items I pointed out for consistency. And please have a follow-up PR for the regression and state stories. Great work here! the drag handle looks simple, but i can see that you have done a great consideration here to make it look and work. 🎉
There was a problem hiding this comment.
Note: once all the PRs are merged for vector, can you please do a smoke test to ensure that the dashed asymptote and focus still works as expected.
There was a problem hiding this comment.
Note: once this is release, please keep an eye out on the translation PR comment in frontend release and request for the new strings based on our process timeline is two weeks after it is requested, it should be still within timeline before our next internal and content play test.
There was a problem hiding this comment.
Please don't forget to add the regression story for this, as a followup PR after all 5 PRs are landed.
There was a problem hiding this comment.
Whoops thought I had one!
There was a problem hiding this comment.
In a followup PR after all 5 PRs are landed please update/added new state behavior here
There was a problem hiding this comment.
| * Arbitrary angles are supported (e.g. for vector alignment). | ||
| * @default 0 | ||
| */ | ||
| rotation?: number; |
There was a problem hiding this comment.
just a comment, this addition is smart. good work! 🎉
| focused: boolean; | ||
| }; | ||
|
|
||
| // useControlArrowhead mirrors useControlPoint but renders a |
There was a problem hiding this comment.
| // useControlArrowhead mirrors useControlPoint but renders a | |
| // Hook that mirrors useControlPoint but renders a |
ivyolamit
left a comment
There was a problem hiding this comment.
Based on our chat, the minimum distance will be something to discuss with content and can be improved later in a followup PR if needed. Adding ✅ 👍🏼
c6ebc40 to
ad02a65
Compare
…and subcomponents.
cee09bc to
8ae7021
Compare




Summary
This PR was created with the help of AI, albeit with heavy oversight and review.
This is part of a series of PRs implementing the vector graph type for the Interactive Graph widget:
PR1 – type definitions and schema
▶️ PR3 – rendering & accessibility (this PR)
PR2 – state management
PR4 – scoring
PR5 – editor support
Issue: LEMS-3971
PillDragHandlecomponent from the asymptote-specific drag handle, now reused by both vector and exponential/logarithm graphsChanges:
New files:
useDraggableon a<g>element with transparent hit target, visible line with rounded caps (thickens on hover/focus/drag via CSS class.movable-vector-line), extension line to arrowhead, and aPillDragHandleat 1/3 along the line.useControlPointhook providing draggable dot with keyboard constraint that prevents overlap with the tail.strokeWidth={2}.onDragEnd) to prevent lingering focus ring.aria-live="polite"for screen reader announcements.describeVectorGraphtests, 3getVectorTipKeyboardConstrainttests.AsymptoteDragHandle. Supports arbitrary rotation angles viarotationprop. Used by vector (rotated to match vector angle) and asymptote (0° or 90°).Modified files:
PillDragHandledirectly instead of the deletedAsymptoteDragHandlewrapper.srVectorGraph,srVectorPoints,srVectorTipPoint,srVectorGrabHandle.renderVectorGraph()dispatch.getVectorEquationString()showing component form⟨dx, dy⟩..movable-vector-line(stroke color, weight, rounded caps) and.pill-drag-handle-*classes. Removed old.movable-asymptote-handle-*classes.VectorStorybook story.Deleted files:
PillDragHandleused directly inMovableAsymptote.Design decisions:
PillDragHandle— Extracted from the asymptote-specific handle so vector, exponential, and logarithm graphs all use the same component. Therotationprop replaces the oldorientationprop (0° = horizontal, 90° = vertical, arbitrary angles for vector).MovableLine, the vector uses the drag handle pill's focus ring for focus indication, avoiding extra SVG lines on tab focus.movable-draggingCSS class when active, but the arrowhead does not (fixedstrokeWidth).--mafs-blue), weight (--movable-line-stroke-weight), and caps (stroke-linecap: round) are in.movable-vector-lineCSS class rather than inline styles.Test plan:
pnpm tsc— no new type errorspnpm test packages/perseus/src/widgets/interactive-graphs/graphs/— all tests pass including 10 vector tests and 6 pill-drag-handle testsPillDragHandlemigration doesn't regress