CODAP-1507: draw legend keys as the category's point shape - #2698
CODAP-1507: draw legend keys as the category's point shape#2698kswenson wants to merge 6 commits into
Conversation
|
Reviewing this stack — six PRs, each based on the one below it, merging bottom-up into
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 four map one-to-one. Nothing here is superseded — this is the top of the stack. Worth your attention, in order:
Out of scope by design: the residual plot, which draws its own SVG circles and is CODAP-1526. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## CODAP-1509-point-shapes-pixi #2698 +/- ##
================================================================
+ Coverage 87.72% 87.94% +0.22%
================================================================
Files 821 821
Lines 47527 47566 +39
Branches 12039 12142 +103
================================================================
+ Hits 41695 41834 +139
+ Misses 5816 5716 -100
Partials 16 16
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
codap-v3
|
||||||||||||||||||||||||||||
| Project |
codap-v3
|
| Branch Review |
CODAP-1507-legend-key-shapes
|
| Run status |
|
| Run duration | 03m 27s |
| Commit |
|
| Committer | Kirk Swenson |
| 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.
🟡 Changes recommended
Map fallback shapes can disagree with rendered points, and remaining Cypress rect selectors will fail.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Updates categorical graph and map legends to represent point shapes accurately.
Changes:
- Adds SVG shape-path generation, sizing, centering, and tests.
- Replaces square categorical swatches with point-shape paths.
- Updates styling and selected Cypress color assertions.
File summaries
| File | Description |
|---|---|
point-shapes.ts |
Adds SVG geometry helpers. |
point-shapes.test.ts |
Tests path generation and fitting. |
legend.scss |
Styles path-based keys. |
categorical-legend.tsx |
Renders categorical keys as shapes. |
graph-legend.spec.ts |
Updates path fill selectors. |
Review details
Suppressed comments (1)
v3/src/components/data-display/components/legend/categorical-legend.tsx:131
- Map legends use the map layer's
displayItemDescription.pointShape, not the top-levelMapContentModel.pointDescription, as the fallback for categories without an assigned shape (map-point-layer.tsx:350-361). As written, a map layer whose base points are stars will show circle keys, and changing that layer's base shape will not update the legend. Resolve the fallback from the layer matching thisdataConfiguration, while retaining the top-level fallback for graphs.
const keyShape = (category: string) =>
dataConfiguration?.getLegendShapeForCategory(
category, displayModel?.pointDescription.pointShape) ?? kDefaultPointShape
- Files reviewed: 5/5 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.
There was a problem hiding this comment.
🟢 Approval recommended
The implementation consistently matches rendered point shapes, handles map layers correctly, and updates affected tests and selectors.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Balanced
7a50b2d to
cf96a45
Compare
6c717ef to
c3eec55
Compare
cf96a45 to
e8f1e90
Compare
An outline as a path, the radius to draw a shape at inside a box, and the center of the drawn box. The radius is the one a shape would be drawn at on a plot, reduced only where that would take it outside the box. The plot radius is shared so that shapes carry about the same visual weight and points can be compared; holding it wherever a box allows keeps that relationship, so a square still reads as lighter than a circle instead of being inflated past it to fill the corners. The pointier shapes cannot be held -- a star is half again as wide as the circle it replaces -- so those give up weight to fit. The box center exists because a triangle and a star sit on their center of area, so their outline reaches further one way than the other. A caller wanting the ink centered in a box takes it out of its placement; a caller drawing a point does not, because there the position is the data. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A categorical key is the category's point shape rather than a square swatch, so
a legend describes a graph that encodes with shape. Without it a reader who
cannot separate the colors gets nothing from three identical squares, which is
the accessibility argument for the whole feature.
Keys with no shape assigned become circles, which is a visible change to every
document with a categorical legend and is deliberately not behind the feature
flag. The square existed to show as much color as possible while every point had
the same shape, and that reasoning does not survive shape carrying data.
A shape keeps its plot radius where the key box allows, and only the ones that
would overflow are scaled down. The square is the shape this is visible on:
scaling it up to fill the box made the lightest of the angular shapes read as
the heaviest key in the set, which is the swatch misreporting rather than merely
looking wrong. The shapes that do have to shrink stay visibly lighter than the
round ones, which is a consequence of a common footprint and worth watching for
reactions.
The box is centered rather than the shape, since a key's position is not data.
The stylesheet selects the swatch by element, so the key shape is listed
alongside: a path would otherwise have lost the border and the softening every
other legend swatch has, without anything failing.
Seven Cypress assertions read the key's fill through `find('rect')` and now read
a path. The numeric legend's helper looks the same but selects the choropleth's
own rects, and is left alone.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Found by Copilot review. Eight more places select a categorical key by element and would have found nothing: three in the graph legend helper, three in the map legend helper, and two in the PIXI interaction spec. The map ones matter most, since nothing else in the suite exercises selecting a category on a map. They were missed because the helpers reach the key through getCategoricalLegendCategory rather than naming it, so searching for the selector did not turn them up. Searching for the element would have. All of these, and the ones already changed, now name `.legend-key-shape` rather than an element, which Copilot also suggested. The numeric and choropleth selectors still want a rect and are untouched, as are the adornment squares, which are unrelated. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Found by Copilot review. A map draws each layer from that layer's description, so a legend reading the tile's own described nothing on screen: a layer whose points were stars showed circle keys, and changing that layer's shape left the legend alone. The description now comes from a view the display answers rather than a property the legend reads, with the map overriding it to find the layer holding the configuration. Asking rather than reading is what keeps the legend from having to know it is on a map, which it must not: data-display cannot branch on the tile above it. The graph is unaffected. It draws every point from one description, which is what the unoverridden view returns. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Naming the key's class rather than its element made these two lines 122 characters. Lint warns rather than fails on length, so they went in unnoticed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The triangle's own geometry explains it. The box center, the radius that fits a box, and the legend key each said it again in their own words. Each of them now says only what it is for, which is the part its caller cannot see: what to do when the box will not take the plot radius, what a caller takes out of a placement, and what the key centers. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
c3eec55 to
e6ae88f
Compare
Draws categorical legend keys as the category's point shape instead of a plain color square. Sixth in the LEADS Point Shapes stack, on top of #2697.
Fixes CODAP-1507.
Why
This is the piece that makes the accessibility argument for the whole feature hold. If shape is a second encoding channel, a legend showing only color does not describe the graph, and a reader who cannot separate three colors gets nothing from three identical squares.
A change every document will see
Keys with no shape assigned become circles, in every document with a categorical legend, on graphs and maps alike. This is deliberately not behind the
pointShapesflag: the same rule as the renderers, where a document authored with shapes must draw them for every reader. The square swatch existed to show as much color as possible while every point had the same shape, and that reasoning does not survive shape carrying data.Worth a reviewer's explicit agreement rather than passing unnoticed, since it changes the appearance of documents whose authors never opted into anything.
Sizing
A shape keeps the radius it would be drawn at on a plot, and is scaled down only where that would take it outside the 15px key box.
The alternative — scaling every shape to fill the box — was built first and rejected: it scales the square up to 127% of the circle's ink, which makes the lightest of the angular shapes read as the heaviest key in the set. That is the swatch misreporting, not just looking wrong. Clamping changes only the square; every other shape already overflows at the shared radius and is scaled down identically either way.
The consequence to look at: a star and a triangle sit at about half the circle's ink, because that is what fitting them into a circle's footprint costs. That spread is visible. It is left as is to see how people react; the lever if it reads badly is a larger key box with more row pitch, not more scaling.
The box is centered rather than the shape, since a triangle and a star sit on their center of area and a key's position — unlike a plotted point's — is not data.
Two things that would have failed silently
legend.scssstyles the swatch with a type selector (rect:not(.attribute-label-bg)), giving it the#315B7Dborder and 0.85 opacity. A path drops out of that with nothing erroring, so the key shape is listed alongside. The numeric and choropleth legends are untouched.find('rect')and now read a path.getNumericLegendCategorieslooks identical but selects the choropleth's own>svg rect, and is deliberately left alone.dis set outside the d3 transition: interpolating one outline into another matches their points up in order, which would animate a change of shape as a scramble.Checked
Graph and map, categorical legends with and without assigned shapes, and dot plots, per the ticket's acceptance criteria.
🤖 Generated with Claude Code