Skip to content

CODAP-1506: add per-category point shape controls - #2693

Open
kswenson wants to merge 4 commits into
CODAP-1505-per-category-point-shapefrom
CODAP-1506-shape-controls
Open

CODAP-1506: add per-category point shape controls#2693
kswenson wants to merge 4 commits into
CODAP-1505-per-category-point-shapefrom
CODAP-1506-shape-controls

Conversation

@kswenson

@kswenson kswenson commented Sep 4, 2026

Copy link
Copy Markdown
Member

Completes CODAP-1506, alongside #2691 which did the structural half. Stacked on
#2692 (the model), which is stacked on #2691 — review those first. I'll retarget
each as its parent merges.

Adds the shape dropdown to every legend category row, and to the single Points
row shown when no legend attribute is assigned. Behind a new pointShapes
feature flag owned by LEADS, so features=LEADS enables the whole set.

Nothing renders a shape yet. Every point still draws as a circle until
CODAP-1504 lands the geometry. The control writes through to the model from
#2692, so the UI is exercisable now and shapes appear once the renderer can draw
them — which is why the UI is landing before the geometry.

The controls

The trigger is icon only: with an icon and a text label the category name is
squeezed to ~55px and a value like "water" truncates. The open menu carries icon
plus label and marks the current shape in bold. Both take their fill from the
category's own colour, so a row's two controls describe the same category.

Trigger geometry mirrors .color-picker-thumb beside it — same padding, same
gap, same 24px inner element — so the two controls in a row are the same size by
construction. Popover values follow the prototype's own .pt-popover /
.pt-listbox.

Assets

Seven SVGs exported from the design prototype, adapted to the repo's icon
convention: fill="currentColor" so rows can tint them, and the decorative
title/role attributes dropped so the control supplies the accessible name.

They use the .nosvgo.svg extension, which keeps SVGR — still React
components, which the currentColor tinting requires — while skipping svgo.
That matters because svgo's removeViewBox drops the viewBox once width/
height are present, and those attributes are what give the glyph intrinsic
dimensions. Without them an svg contributes nothing to its parent's intrinsic
width, and the trigger sized itself to padding + arrow alone.

Note the sibling .nosvgr.svg is a different rule — that one yields a URL
asset, which can't inherit color.

Two things reviewers should know

Glyphs are pinned, not laid out. Each sits in a plain sized box with
position: absolute; inset: 0. Laid out as flex items, the svgs rendered
outside their parent's box — measured 3px above and left of it in the trigger,
and all seven stacked at one point in the menu. I could not identify the cause
from the stylesheet, the DOM, or the served bundle; taking them out of the flex
layout path removes the failure rather than explaining it. Flagging it as
containment rather than a root-cause fix.

shouldFlip={false} is a workaround. A menu react-aria decides to flip
above its trigger doesn't render at all, which makes the control unusable rather
than awkward: with the palette near the bottom of the window there's no room
below, so the menu could never be opened. Pinned below, it always appears and
scrolls within its max-height. The colour picker in this same palette disables
flipping for the same reason, so the fault is in how popovers position within
the palette rather than in either control. Filing separately; the code comment
says to remove shouldFlip once it's fixed.

Row alignment

The category row distributes with space-between, so a control following the
label tracked the label's width and the controls staggered between rows. The
label now absorbs the free space and truncates, pinning both controls to the
right edge and giving the category name the width the panel was widened for.

Testing

11 new tests. Mutation-verified — each of these fails at least one: ignoring the
feature flag, forcing it off, writing a shape to the wrong category, letting
SelectValue render the selected item's children (which duplicated the glyph),
and dropping the hidden value that gives the trigger its accessible name.

Manually verified in the browser across the palette: trigger and menu geometry,
per-category tinting, keyboard operation, and that the menu escapes the category
scroll container rather than being clipped — the AC that called this out as
"the first menu CODAP opens from inside that scrolling list".

npm run build:tsc, npm run lint, and the full suite (3814 tests / 357 suites)
all pass.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Ctmv18kjkPWnUq5bek5nrH

@kswenson kswenson added the v3 CODAP v3 label Sep 4, 2026
@codecov

codecov Bot commented Sep 4, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 93.44262% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.87%. Comparing base (256bc48) to head (bfddeae).

Files with missing lines Patch % Lines
...s/data-display/inspector/legend-color-controls.tsx 92.00% 2 Missing ⚠️
...nts/data-display/inspector/point-shape-setting.tsx 94.44% 2 Missing ⚠️
Additional details and impacted files
@@                           Coverage Diff                           @@
##           CODAP-1505-per-category-point-shape    #2693      +/-   ##
=======================================================================
+ Coverage                                87.85%   87.87%   +0.01%     
=======================================================================
  Files                                      819      820       +1     
  Lines                                    47279    47338      +59     
  Branches                                 12085    12097      +12     
=======================================================================
+ Hits                                     41536    41597      +61     
+ Misses                                    5729     5727       -2     
  Partials                                    14       14              
Flag Coverage Δ
cypress 69.69% <34.78%> (+<0.01%) ⬆️
jest 63.09% <93.44%> (+0.03%) ⬆️

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

☔ View full report in Codecov by Harness.
📢 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

cypress Bot commented Sep 4, 2026

Copy link
Copy Markdown

codap-v3    Run #12205

Run Properties:  status check passed Passed #12205  •  git commit bfddeae469: null
Project codap-v3
Branch Review CODAP-1506-shape-controls
Run status status check passed Passed #12205
Run duration 09m 07s
Commit git commit bfddeae469: null
Committer null
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 82
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 384
View all changes introduced in this branch ↗︎

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Shape controls are incorrectly exposed for map polygon layers where they have no effect.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds feature-gated point-shape controls to data-display formatting, using the shape model from #2692; point rendering remains deferred.

Changes:

  • Adds per-category and no-legend shape selectors.
  • Adds seven tintable SVG shape icons and styling.
  • Registers the feature flag, translations, and tests.
File summaries
File Description
en-US.json5 Adds shape labels.
feature-flag-registry.ts Registers pointShapes.
point-shape-setting.tsx Implements the shape selector.
legend-color-controls.tsx Integrates shape controls.
legend-color-controls.test.tsx Tests gating and selection.
display-item-format-control.scss Styles controls and rows.
point-circle.nosvgo.svg Adds circle glyph.
point-diamond.nosvgo.svg Adds diamond glyph.
point-plus.nosvgo.svg Adds plus glyph.
point-square.nosvgo.svg Adds square glyph.
point-star.nosvgo.svg Adds star glyph.
point-triangle.nosvgo.svg Adds triangle glyph.
point-x.nosvgo.svg Adds X glyph.
Review details

Suppressed comments (1)

v3/src/components/data-display/inspector/legend-color-controls.tsx:210

  • Categorical map polygon layers reach this branch too, so enabling the flag adds a point-shape selector for every polygon category and writes shape metadata with no visible effect. Compute shape availability before the categorical return, include the polygon sentinel check, and pass that value into CategoricalColorControls instead of checking only the flag here.
          <If condition={isFeatureEnabled("pointShapes")}>
  • Files reviewed: 6/13 changed files
  • Comments generated: 1
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread v3/src/components/data-display/inspector/legend-color-controls.tsx Outdated
kswenson and others added 2 commits September 8, 2026 16:38
Add the shape dropdown to each legend category row, and to the single Points
row shown when no legend attribute is assigned. Gated behind a new pointShapes
feature flag owned by LEADS, so `features=LEADS` turns the whole set on.

Nothing renders a shape yet -- every point still draws as a circle until
CODAP-1504 lands the geometry. The control writes through to the model from
CODAP-1505, so the UI is exercisable now and the shapes appear when the
renderer learns to draw them.

The trigger is icon only: with an icon and a text label the category name is
squeezed to about 55px and a value like "water" truncates. The open menu carries
icon plus label, and marks the current shape in bold. Both take their fill from
the category's own color, so a row's two controls describe the same category.

Assets
------

Seven SVGs exported from the design prototype, adapted to the repo's icon
convention: fill is currentColor so a row can tint them, and the decorative
title/role attributes are dropped so the control supplies the accessible name.

They use the .nosvgo.svg extension, which keeps SVGR (still React components,
as the tinting requires) while skipping svgo. That matters because svgo's
removeViewBox drops the viewBox once width and height are present, and those
attributes are what give the glyph intrinsic dimensions. Without them the svg
contributes nothing to its parent's intrinsic width, and the trigger sizes
itself to padding plus arrow alone.

Layout
------

Trigger geometry mirrors .color-picker-thumb beside it -- same padding, same
gap, same 24px inner element -- so the two controls in a row are the same size
by construction rather than by coincidence. Popover values follow the
prototype's own .pt-popover and .pt-listbox.

Each glyph is pinned inside a plain box with inset: 0 rather than placed by
flex layout. Laid out normally, the svg rendered outside its parent's box --
measured 3px above and left of it in the trigger, and stacked at a single point
in the menu. The cause was not identified; taking the glyph out of the flex
layout path removes the failure rather than explaining it.

Note that a plain className on a react-aria component REPLACES its default
class, so the trigger carries point-shape-thumb and not react-aria-Button. The
stylesheet says so, because adding .react-aria-Button to those selectors would
silently match nothing.

Row alignment
-------------

The category row distributes with space-between, so a control following the
label tracked the label's width and the controls staggered from row to row. The
label now absorbs the free space and truncates, which pins both controls to the
right edge and gives the category name the width the panel was widened for.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Ctmv18kjkPWnUq5bek5nrH
Work around a popover bug: a menu react-aria decides to flip above its trigger
does not render at all. That leaves the control unusable rather than merely
awkward -- with the palette near the bottom of the window there is no room
below, so the menu could never be opened.

Disabling shouldFlip keeps it below the trigger, where it always appears, and a
max-height lets it scroll within itself rather than run off the bottom of the
window. Clipping is the lesser failure.

The colour picker in this same palette disables flipping for the same reason,
so the fault lies in how popovers position within the palette rather than in
either control. The comment says to remove shouldFlip once that is fixed.

Also compare closeTrigger against its previous value rather than firing on
every render, so mounting no longer counts as a scroll and closes the menu.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Ctmv18kjkPWnUq5bek5nrH
The map mounts these controls for its polygon layers as well as its point
layers, and a polygon layer marks itself with a negative point size -- the same
sentinel that already hides the Point Size slider. The shape gates ignored it,
so a polygon layer's fill row was relabeled "Points" and offered a shape that
cannot reach polygon rendering.

Decided once where the display description is in hand and passed down, since the
per-category gate is in a child component that does not receive it.

The test's mock description had no point size at all, which the real model
always has, so completing the mock was part of the fix rather than a
consequence of it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Ctmv18kjkPWnUq5bek5nrH

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

The controls are correctly gated, accessible, model-connected, polygon-safe, and adequately tested.

Review details
  • Files reviewed: 6/13 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

@kswenson

kswenson commented Sep 9, 2026

Copy link
Copy Markdown
Member Author

Reviewing this stack — four PRs, each based on the one below it, merging bottom-up into leads-point-shapes:

PR Story What it is
#2691 CODAP-1506 (part 1) Format palette restructure — structural, no shape behavior
#2692 CODAP-1505 Per-category shape storage — model only
#2693 CODAP-1506 (part 2) The shape controls — UI
#2696 CODAP-1504 Canvas rendering and hit testing

CODAP-1506 is split across #2691 and #2693, so approving #2691 does not complete that story — it should not leave code review until #2693 is approved too. The other two map one-to-one.

Some things are introduced early and deliberately revised further up the stack. Rather than have you spend time on them twice, each PR notes what is superseded.

Superseded in #2696 — no need to flag these here:

The model code visible as context here is #2692's, so the three items noted on that PR apply to it.

Worth your attention here: the react-aria Select wiring in point-shape-setting.tsx. Two non-obvious things are load-bearing and commented — shouldFlip={false} works around CODAP-1510 (palette popovers don't render when they need to flip above the trigger), and the trigger carries point-shape-thumb rather than react-aria-Button because a plain className replaces react-aria's default class.

Out of scope by design, each with a ticket: PIXI renderer (CODAP-1509), legend keys (CODAP-1507), residual plot (CODAP-1526).

…l shape in inspector with color gradient representing numeric legend range.
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.

2 participants