Skip to content

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

Merged
kswenson merged 6 commits into
leads-point-shapesfrom
CODAP-1506-shape-controls
Sep 11, 2026
Merged

CODAP-1506: add per-category point shape controls#2693
kswenson merged 6 commits into
leads-point-shapesfrom
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.75000% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.86%. Comparing base (256bc48) to head (8becf91).
⚠️ Report is 5 commits behind head on leads-point-shapes.

Files with missing lines Patch % Lines
...s/data-display/inspector/legend-color-controls.tsx 92.30% 2 Missing ⚠️
...nts/data-display/inspector/point-shape-setting.tsx 94.44% 2 Missing ⚠️
Additional details and impacted files
@@                 Coverage Diff                 @@
##           leads-point-shapes    #2693   +/-   ##
===================================================
  Coverage               87.85%   87.86%           
===================================================
  Files                     819      820    +1     
  Lines                   47279    47341   +62     
  Branches                12085    12097   +12     
===================================================
+ Hits                    41536    41594   +58     
- Misses                   5729     5731    +2     
- Partials                   14       16    +2     
Flag Coverage Δ
cypress 69.67% <38.77%> (-0.02%) ⬇️
jest 63.09% <93.75%> (+0.04%) ⬆️

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 #12240

Run Properties:  status check passed Passed #12240  •  git commit b1ec39b21a: Merge pull request #2693 from concord-consortium/CODAP-1506-shape-controls
Project codap-v3
Branch Review leads-point-shapes
Run status status check passed Passed #12240
Run duration 03m 40s
Commit git commit b1ec39b21a: Merge pull request #2693 from concord-consortium/CODAP-1506-shape-controls
Committer Kirk Swenson
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 ↗︎

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.
Only the trigger took the gradient, so the menu's options kept the display's own
point color. That color stops describing anything once a numeric legend governs
the plot: a shape chosen purple before the legend was applied left seven purple
options in a plot with no purple points in it.

Each option previews the points that choosing it would produce, which is what
the menu already does elsewhere -- the display's color with no legend, the
category's color in a categorical row. With a numeric legend that preview is the
range, the same one the trigger shows.

The trigger and the options now share one style, so the two cannot drift.

The rule routing a glyph's fill through the custom property moves out of the
trigger's block to cover both. The menu is rendered in a portal, outside the
palette the trigger is scoped to, so a rule nested there could not reach it and
the property would have had no effect on an option.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
kswenson added a commit that referenced this pull request Sep 10, 2026
…abelled sections

Groups the palette's controls under Data Points and Graph headings, and widens the palette to fit them. The map layers palette opts out: it repeats these controls per layer under each layer's own name, so a heading inside each would be noise.

Structural only -- no shape behavior. First of the LEADS Point Shapes stack.

Part of CODAP-1506, which is completed by #2693.
Base automatically changed from CODAP-1505-per-category-point-shape to leads-point-shapes September 10, 2026 16:27

@tealefristoe tealefristoe 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.

Looks good 👍

Comment thread v3/src/components/data-display/inspector/display-item-format-control.scss Outdated
Comment thread v3/src/components/data-display/inspector/display-item-format-control.scss Outdated
Comment thread v3/src/components/data-display/inspector/display-item-format-control.scss Outdated
Comment thread v3/src/components/data-display/inspector/display-item-format-control.scss Outdated
Comment thread v3/src/components/data-display/inspector/point-shape-setting.tsx Outdated
Comment thread v3/src/components/data-display/inspector/point-shape-setting.tsx Outdated
Comment thread v3/src/components/data-display/inspector/point-shape-setting.tsx Outdated
Comment thread v3/src/components/data-display/inspector/point-shape-setting.tsx Outdated
Comment thread v3/src/components/data-display/inspector/legend-color-controls.test.tsx Outdated
Name the polygon sentinel. A negative point size is how a polygon layer says it
has no point, and two places compared against it directly with a comment saying
what the comparison meant. `isPolygon` says it instead, and the shape gate and
the Point Size slider now ask the same question -- which is the thing they were
not doing when one of them offered a shape a polygon could not take.

Keep the palette thumbs in sync in code rather than in a comment. The color
picker's and the shape control's triggers repeated a dozen declarations with a
comment on one saying it mirrored the other; both include a mixin now. Same for
the glyph box and the glyph inside it, which the trigger and the menu items each
declared separately.

That left a comment describing padding the rule does not have -- it claimed less
padding on the left, over a uniform 3px -- which is the kind of drift the mixin
exists to prevent.

Answer in code why one thumb needs `!important` and the other does not: the
color picker's trigger is a react-aria Button and inherits rules that set its
height and padding, while the shape control passes a plain className, which
replaces that class.

Define `hasFillGradient` once rather than testing the same condition twice.

Drop the references to "the prototype", which named something a reader of this
file cannot reach, and the comments that described the markup beneath them.

Rewrite the note on the hidden SelectValue, whose first sentence buried what the
element is for.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@kswenson
kswenson merged commit b1ec39b into leads-point-shapes Sep 11, 2026
27 checks passed
@kswenson
kswenson deleted the CODAP-1506-shape-controls branch September 11, 2026 02:58
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.

3 participants