Skip to content

chore: drag overlay style updates and accessibility improvements (CODAP-566)#2514

Merged
emcelroy merged 4 commits intomainfrom
CODAP-566-click-to-pickup-attributes
Apr 17, 2026
Merged

chore: drag overlay style updates and accessibility improvements (CODAP-566)#2514
emcelroy merged 4 commits intomainfrom
CODAP-566-click-to-pickup-attributes

Conversation

@emcelroy
Copy link
Copy Markdown
Contributor

@emcelroy emcelroy commented Apr 14, 2026

CODAP-566

Updates attribute drag overlay and drop zone styles to match new design spec. Also implements some accessibility improvements in support of keyboard dragging.

  • Updates drag overlay (preview element) with new colors, border, and drag-indicator icon
  • Adds borders to graph drop zones with outline-based rendering and 1px gaps between adjacent zones
  • Sizes graph drop zones using plot bounds so they align with the plot area rather than the full container
  • Adds dnd-kit screen reader announcements for drag start, end, and cancel events — screen readers now announce attribute name (e.g., "Picked up attribute Height") instead of the raw internal drag ID
  • Adds localized aria-roledescription to draggable attribute elements
  • Disables table scrolling during attribute drags -- prevents scrolling of table during keyboard drags

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates CODAP’s drag-and-drop styling and accessibility, focusing on clearer drop-zone highlighting, better-aligned graph drop zones, and improved screen reader output during drags.

Changes:

  • Adds localized screen reader announcements and aria-roledescription for draggable attributes.
  • Restyles drag overlay preview and standardizes drop-zone highlight styles across graph/map overlays.
  • Adjusts graph drop zone sizing/alignment using plot bounds and adds plot drop-zone insets to avoid overlap with adjacent zones.
  • Disables case table scrolling during attribute drags to prevent keyboard drag interactions from scrolling the table.

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
v3/src/utilities/translation/lang/en-US.json5 Adds localized strings for DnD roledescription + announcements.
v3/src/lib/dnd-kit/codap-dnd-context.tsx Adds dnd-kit accessibility announcements for drag start/end/cancel.
v3/src/hooks/use-drag-drop.ts Adds localized aria-roledescription for draggable attributes.
v3/src/components/vars.scss Introduces shared drop-zone-states mixin + highlight token.
v3/src/components/graph/graphing-types.ts Adds shared TS constants for drop zone sizing/gaps.
v3/src/components/graph/components/graph.tsx Computes plot drop-zone insets and passes right-zone presence to add-attribute droppables.
v3/src/components/graph/components/graph.scss Updates add-attribute drop zone dimensions + uses shared mixin.
v3/src/components/graph/components/droppable-plot.tsx Plumbs optional insets through to DroppableSvg.
v3/src/components/graph/components/droppable-add-attribute.tsx Sizes add-attribute drop zones using plot bounds + 1px gap behavior.
v3/src/components/drag-drop/attribute-drag-overlay.tsx Adds drag-indicator icon to the attribute drag overlay.
v3/src/components/drag-drop/attribute-drag-overlay.scss Restyles the drag overlay to match updated visual specs.
v3/src/components/data-display/components/droppable-svg.tsx Adds IDropInsets and applies inset math to overlay bounds.
v3/src/components/data-display/components/droppable-svg.scss Switches to shared drop-zone highlight mixin.
v3/src/components/case-table/case-table.tsx Disables case-table scrolling during attribute drags via a .dragging class.
v3/src/components/case-table/case-table.scss Implements scroll disabling via .case-table.dragging * { overflow: hidden !important; }.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread v3/src/components/graph/components/graph.tsx
Comment thread v3/src/components/graph/components/droppable-add-attribute.tsx Outdated
Comment thread v3/src/components/case-table/case-table.tsx Outdated
Comment thread v3/src/lib/dnd-kit/codap-dnd-context.tsx
Comment thread v3/src/lib/dnd-kit/codap-dnd-context.tsx Outdated
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 14, 2026

Codecov Report

❌ Patch coverage is 93.58974% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.91%. Comparing base (f466322) to head (cc4cdf1).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
v3/src/lib/dnd-kit/codap-dnd-context.tsx 81.25% 3 Missing ⚠️
v3/src/components/case-table/case-table.tsx 85.71% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2514      +/-   ##
==========================================
+ Coverage   86.90%   86.91%   +0.01%     
==========================================
  Files         785      785              
  Lines       44388    44457      +69     
  Branches    11136    10746     -390     
==========================================
+ Hits        38577    38642      +65     
- Misses       5798     5804       +6     
+ Partials       13       11       -2     
Flag Coverage Δ
cypress 69.93% <92.42%> (+0.04%) ⬆️
jest 59.50% <29.48%> (-0.06%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@cypress
Copy link
Copy Markdown

cypress bot commented Apr 14, 2026

codap-v3    Run #11035

Run Properties:  status check passed Passed #11035  •  git commit 3f595c1dc1: Increment the build number
Project codap-v3
Branch Review main
Run status status check passed Passed #11035
Run duration 02m 10s
Commit git commit 3f595c1dc1: Increment the build number
Committer github-actions[bot]
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 4
View all changes introduced in this branch ↗︎

@emcelroy emcelroy marked this pull request as ready for review April 14, 2026 23:35
@emcelroy emcelroy requested review from dougmartin and kswenson April 14, 2026 23:35
Copy link
Copy Markdown
Member

@dougmartin dougmartin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍

"V3.CaseTable.attributeAriaLabel": "Attribute %@, open menu",
"V3.CaseTable.attributeDragInstructions": "Press Space to drag the attribute within the table or to a graph. Press Enter to open the attribute menu.",
"V3.DnD.draggableAttribute": "draggable attribute",
"V3.DnD.onDragStart": "Picked up attribute %@.",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really a code question, but is there a reason this isn't "Dragging attribute %@.", since click-to-pickup hasn't been implemented.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kswenson Sorry, almost missed this. I changed it to "Dragging attribute %@" in cc4cdf1. I was focused on the Spacebar to initiate drag behavior which is more like a pick up, but "Dragging attribute %@" fits that well enough and the mouse dragging behavior better, too.

Copy link
Copy Markdown
Member

@kswenson kswenson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good -- the following review, developed in conjunction with Claude Code 🤖, identifies a couple of issues worth considering. The horizontal dragging issue seems worth fixing. The dynamically-adjustable drop zones on narrow graphs should be logged as a Jira story if not addressed here.

PR Review Summary

Changes: 15 files, +185 / -65 lines

What it does

Updates the attribute drag overlay and drop-zone styles to match a new Zeplin design, and bundles several keyboard/a11y improvements for attribute dragging. New overlay uses accent border, drop shadow, and a drag-indicator icon; graph add-attribute drop zones get outline-based highlighting and are sized to the plot bounds with a 1px gap between adjacent zones; the table is prevented from scrolling during attribute drags; and dnd-kit now emits localized screen-reader announcements that include the attribute name instead of the raw drag ID.

The approach

  • Shared SCSS mixin (vars.scss): new drop-zone-states mixin centralizes the .active/.over styling previously copy-pasted in graph.scss and droppable-svg.scss. Good consolidation.
  • Plot-bound drop zones: DroppableAddAttribute (droppable-add-attribute.tsx:55) now reads layout.getComputedBounds("plot") and constrains its dimensions to the plot area, inset by kDropZoneGap. DroppablePlot accepts an IDropInsets prop forwarded through DroppableSvg so the plot drop zone shrinks to leave room for neighboring zones.
  • Conditional inset calculation (graph.tsx:399-419): Graph uses useDndContext() to read the active drag, then asks placeCanAcceptAttributeIDDrop whether the top/right zones would actually accept this attribute, and only insets the plot drop zone when they would. This avoids visual gaps when no add-attribute zone is showing.
  • Table scroll suppression (case-table.tsx:49-67): useDndMonitor toggles a dragging class on the root via ref; .dragging * forces overflow: hidden !important on all descendants. Direct DOM class manipulation (rather than React state) avoids triggering a re-render of the whole table on every drag.
  • Screen reader support (codap-dnd-context.tsx:36-52): custom Announcements object returns localized strings with the attribute name for attribute drags, and returns undefined otherwise so dnd-kit's defaults apply to non-attribute drags. onDragOver returns undefined to suppress dnd-kit's per-pixel spam. aria-roledescription="draggable attribute" added via useDraggableAttribute.

Assessment

Clean, focused PR. The abstractions pay for themselves (shared mixin, IDropInsets prop, single announcement handler). Key design choices that merit attention:

  1. kDropZoneSize duplicated in SCSS: graph.scss hardcodes 50px for top/yPlus/right zones while graphing-types.ts exports kDropZoneSize = 50. The code comment flags this. Minor drift risk, but acceptable given SCSS/TS boundary.
  2. useDndContext() in Graph: reads the full dnd context on every render. active is reference-stable during a drag, so this triggers renders on drag start/end, not per pixel — fine.
  3. Direct DOM class toggling on the table: unusual for a React component but intentional (performance). tableRef survives because the same element is owned for the tile's lifetime.
  4. tabIndex: -1 override: useDraggableAttribute still forces tabIndex: -1 (RDG compatibility); keyboard drag is initiated elsewhere. Not regressed by this PR.
  5. aria-hidden="true" on the drag-indicator icon: correct — the attribute name is the label.

No test changes, which is appropriate for this visual/a11y polish PR — the behavior changes are in CSS/DOM and the screen-reader text.

Issues

  1. Table .dragging * blocks horizontal scroll needed to reorder attributes. case-table.scss uses overflow: hidden !important, which kills both axes. During an attribute drag within a wide table, users need horizontal scrolling to reach a drop target that's off-screen horizontally (drag-to-reorder within the table). Please change to overflow-y: hidden !important so vertical scroll is suppressed (the original motivation) while horizontal scroll is preserved.
  2. Fixed 50px drop-zone size is cramped on narrow graphs. In the standard Mammals document, the middle (LifeSpan) graph is ~115px wide. Adding a numeric X attribute to make a scatter plot leaves the yPlus (left) and rightNumeric (right) zones at 50px each, leaving only ~15px for the plot drop zone. The size should probably scale down (or the add-attribute zones should cap at some fraction of the plot width) when space is tight. OK to leave as a future refinement, but worth noting.

Minor notes:

  • The SCSS/TS drop-zone-size duplication would be marginally safer as a CSS custom property or SCSS variable consumed from TS. Leave as-is unless convenient.
  • hasRightDropZone is computed in Graph and passed as a prop to each DroppableAddAttribute; each child already calls useGraphLayoutContext. Fine, but if the plot inset logic grows, consider hoisting more of it.

@emcelroy
Copy link
Copy Markdown
Contributor Author

emcelroy commented Apr 15, 2026

👍 Looks good -- the following review, developed in conjunction with Claude Code 🤖, identifies a couple of issues worth considering. The horizontal dragging issue seems worth fixing. The dynamically-adjustable drop zones on narrow graphs should be logged as a Jira story if not addressed here.

@kswenson Thanks for catching the horizontal scroll issue. That's fixed in 41acbc2.

I'm going to have to skip the dynamically-adjustable drop zones issue, but I made a Jira story for it.

@emcelroy emcelroy requested a review from kswenson April 15, 2026 21:08
@emcelroy emcelroy force-pushed the CODAP-566-click-to-pickup-attributes branch from 8b46366 to cc4cdf1 Compare April 15, 2026 21:37
@emcelroy emcelroy merged commit 93bd4e4 into main Apr 17, 2026
27 checks passed
@emcelroy emcelroy deleted the CODAP-566-click-to-pickup-attributes branch April 17, 2026 15:46
Copy link
Copy Markdown
Member

@kswenson kswenson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Changes LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants