Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2513 +/- ##
=======================================
Coverage 86.87% 86.88%
=======================================
Files 784 784
Lines 44308 44370 +62
Branches 11112 11121 +9
=======================================
+ Hits 38493 38549 +56
- Misses 5804 5808 +4
- Partials 11 13 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
codap-v3
|
||||||||||||||||||||||||||||
| Project |
codap-v3
|
| Branch Review |
main
|
| Run status |
|
| Run duration | 02m 07s |
| Commit |
|
| Committer | github-actions[bot] |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
|
|
0
|
|
|
0
|
|
|
0
|
|
|
0
|
|
|
4
|
| View all changes introduced in this branch ↗︎ | |
There was a problem hiding this comment.
Pull request overview
Implements updated graph axis/legend attribute label UI so labels look like visible dropdown controls per CODAP-1137, including new SVG background/arrow styling and layout adjustments.
Changes:
- Adds SVG label background/arrow rendering and new hover/focus/selected styling for axis/legend attribute labels.
- Adjusts axis extents/label spacing and axis render order to prevent label/background overlap.
- Updates menu styling and multiple tests to align with the redesigned UX and new default graph size.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| v3/src/utilities/translation/lang/en-US.json5 | Updates the empty-graph cue string. |
| v3/src/components/vars.scss | Increases empty label font size to match design. |
| v3/src/components/graph/graph-registration.ts | Updates default graph tile dimensions. |
| v3/src/components/graph/components/graph.tsx | Renders horizontal axes first so vertical labels stay on top. |
| v3/src/components/graph/components/graph.scss | Updates empty-label fill color to use shared font color. |
| v3/src/components/graph/components/graph-axis.tsx | Ensures horizontal axis background spans full graph width. |
| v3/src/components/graph/components/graph-attribute-label.tsx | Adds SVG background/arrow behind axis labels and tile-selected class styling. |
| v3/src/components/data-display/components/legend/legend.scss | Allows legend overflow and avoids styling the new label background rect. |
| v3/src/components/data-display/components/legend/legend-attribute-label.tsx | Adds SVG background/arrow and tile-selected styling for legend label. |
| v3/src/components/data-display/components/attribute-label.scss | Adds styles for label background + dropdown arrow + interaction states. |
| v3/src/components/axis/hooks/use-axis.ts | Adjusts desired axis extent to add whitespace around labels via labelMargin. |
| v3/src/components/axis/components/axis-or-legend-attribute-menu.tsx | Relays hover/focus/menu-open classes to SVG target and updates aria-label behavior. |
| v3/src/components/axis/components/axis-or-legend-attribute-menu.test.tsx | Updates aria-label test and adds class-relay tests. |
| v3/src/components/axis/components/axis-or-legend-attribute-menu.scss | Updates Chakra MenuList/menuitem styling and overlay layout rules. |
| v3/src/components/axis/axis-utils.ts | Adds shared renderLabelBackground() helper for SVG rect + arrow rendering. |
| v3/src/components/axis/axis-types.ts | Introduces labelMargin and labelPaddingX constants. |
| v3/cypress/support/helpers/axis-helper.ts | Updates expected default axis cue text. |
| v3/cypress/e2e/graph.spec.ts | Fixes a selector typo. |
| v3/cypress/e2e/axis.spec.ts | Tightens selectors for axis label assertions with multi-y labels. |
Comments suppressed due to low confidence (2)
v3/src/components/axis/components/axis-or-legend-attribute-menu.tsx:394
- The main attribute menu no longer has a max height or scroll overflow. With many attributes/collections this can grow past the viewport/container and become unusable. Consider restoring maxH/overflowY (ideally using useMenuHeightAdjustment on mainMenuListRef, like the submenus) so the main menu remains scrollable and constrained.
<Portal containerRef={containerRef}>
<MenuList ref={mainMenuListRef} className="axis-legend-menu"
onFocus={handleMenuItemFocus}
onKeyDown={handleMainMenuKeyDown}
data-testid={`axis-legend-attribute-menu-list-${place}`}>
{renderMenuItems()}
v3/src/components/graph/components/graph-attribute-label.tsx:204
- updateTextSelection sets the same attributes twice (e.g. 'class' and 'data-testid' are each applied two times). This is redundant and makes future changes error-prone; consider removing the duplicates so the attribute list is single-sourced.
return selection
.attr('class', className)
.attr('text-anchor', 'middle')
.attr('dominant-baseline', 'central')
.attr('data-testid', className)
.attr("transform", labelTransform + tRotation)
.attr('class', className)
.attr('data-testid', className)
.style('visibility', visibility)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const refreshAxisTitle = useCallback(() => { | ||
|
|
||
| const updateSelection = (selection: any) => { | ||
| const updateTextSelection = (selection: any) => { |
There was a problem hiding this comment.
UC said the type on selection could be tightened to d3.Selection<SVGTextElement, number, any, any> from any with the same update in in axis-utils.ts:389 and :404
There was a problem hiding this comment.
Thanks. I found that wasn't really possible, though. d3's .call() passes different Selection types from enter (lines 238-240) and update (line 242), making it difficult to type a shared function without any. I spent a few minutes with UC trying to work that out, but the change needed was relatively substantial. I think the pre-existing any was a pragmatic choice, so I'm going to leave it. I ran into similarly complex issues with d3 types in axis-utils.ts.
kswenson
left a comment
There was a problem hiding this comment.
👍 Looks good -- new design is clean when not directly interacting with the label. The following review, developed in conjunction with Claude Code 🤖, contains a few issues/observations/suggestions to consider, but I leave it to your discretion which/whether to address them.
PR Review Summary
Changes: 21 files, +350 / -85 lines
What it does
Implements CODAP-1137, making the attribute-picker dropdown on graph axes and legends visibly discoverable for accessibility. Each attribute label now renders inside an SVG background rect with a dropdown caret and four visual states (resting/hover/focus/menu-open). Per Daniel Damelin's 2026-04-10 decision, the caret is persistent when the tile is selected and only appears on hover when it is not — the "ideal" (not fallback) specification.
The approach
- Reusable SVG background renderer: New
renderLabelBackground()inaxis-utils.tsdraws arect.attribute-label-bgandsvg.attribute-label-arrowbehind/after a text selection. Shared byGraphAttributeLabel,SingleYAttributeLabel, andLegendAttributeLabel. - State relay: The React
AxisOrLegendAttributeMenuoverlay relayshovered/focused/menu-open/tile-selectedas CSS classes onto the target<g>. All styling is driven from those classes inattribute-label.scss. This is a clean separation — the React overlay owns interaction, the SVG owns visuals. - Geometry: Adds
labelMargin(13) andlabelPaddingX(8). Adoptsdominant-baseline: centralfor predictable vertical centering of the text inside the rect, simplifyingtX/tYcalculations ingraph-attribute-label.tsx. - Axis render order:
graph.tsxsorts axes horizontal-first so vertical axis labels render on top of horizontal axis backgrounds, avoiding clipping. - Default graph size: 300×300 → 340×340 (
graph-registration.tsandadd-default-content.ts), to give the new padded labels more room. - Cleanup: Removes the old absolute-positioned HTML
DropdownArrow, deletes obsolete SCSS for the per-place arrow rotation, and simplifies legend<rect>styling to exempt the new.attribute-label-bg.
Assessment
Clean, well-structured implementation that matches the ideal path of the approved design. Test coverage is strong: five new Jest tests in axis-or-legend-attribute-menu.test.tsx cover the hover/focus/menu-open/unmount class-relay behavior, and the Cypress tests were updated for renamed data-testids and the new button/label count shape. The factored-out renderLabelBackground is a nice touch and keeps the three call sites consistent.
The design decision to drive visuals via CSS classes on a <g> works well and keeps the imperative D3/React boundary tidy. The dominant-baseline: central centering is a worthwhile simplification.
Issues
Minor observations — none blocking:
-
Focus-class race in
useEffectcleanup. TheuseEffecton[isMenuOpen, target]removes thefocusedclass whenisMenuOpenflips to false. If focus has already returned toMenuButtonby then (common after Escape/click-item close), the onFocusCapture handler on a child should re-add it — but the interaction is order-dependent on React's commit/focus sequence. Likely fine in practice; worth a quick manual check with keyboard-only navigation. -
Adornment test tolerance widened.
adornments.spec.ts:464expands from(271, 273)to(260, 275)to accommodate the size change. Wider tolerance weakens the assertion. Acceptable given the default-size change, but worth noting. -
Loose
anycast in the d3join(enter => enter.append('rect'))callbacks insiderenderLabelBackground(any). Consistent with other d3 usage in the file, but not necessary here. -
Undocumented
!importantonbutton { position: relative }inaxis-or-legend-attribute-menu.scss. The other!importants in this file are documented as Chakra overrides; this one is not. Two questions worth confirming: (a) is the rule still needed at all? It looks like a leftover from when the dropdown caret was an absolutely-positioned HTML child of.attribute-label-menuand needed a containing block — the caret is now inside SVG, so this may do nothing. (b) If the rule is still needed, is!importantactually required? Chakra'sMenuButtonstyles come from emotion classes (.chakra-menu__menu-button) that are single-class selectors and shouldn't win over.attribute-label-menu button. If!importantis genuinely needed, prefer bumping specificity (e.g..attribute-label-menu button.chakra-menu__menu-button { position: relative }) and add a comment matching the pattern used further down the file. -
Caret path is hard-coded inline, duplicating
arrow.svg.renderLabelBackgrounddoesenter.append('svg').append('path').attr('d', 'm12 15-5-5h10z')— the same glyph as the now-removedDropdownArrowimport fromv3/src/assets/icons/arrow.svg. The inline approach is understandable (D3's imperative append chain can't consume an SVGR-produced React component, and the caret needs to live inside the D3-managed<g>so the CSS-class state styling works). But we now have two sources of truth for the same glyph. Options: (a) extract the path string into a named constant with a comment pointing atarrow.svg, cheapest fix; (b) adopt a<symbol>/<use>pattern so the asset can be the single source — more plumbing, cleanest result. Not a blocker, but worth addressing before design tweaks the caret.
Overall this is a substantive, thoughtful PR that lands the decided-upon behavior with good tests.
CODAP-1137
Redesigns axis and legend attribute labels on graphs to match updated design spec.
labelMargin