Skip to content

CODAP-1526: draw residual points with the category's shape - #2699

Open
kswenson wants to merge 1 commit into
CODAP-1507-legend-key-shapesfrom
CODAP-1526-residual-plot-shapes
Open

CODAP-1526: draw residual points with the category's shape#2699
kswenson wants to merge 1 commit into
CODAP-1507-legend-key-shapesfrom
CODAP-1526-residual-plot-shapes

Conversation

@kswenson

Copy link
Copy Markdown
Member

Draws residual-plot points with the category's shape instead of always a circle. Seventh in the LEADS Point Shapes stack, on top of #2698.

Fixes CODAP-1526.

Why this exists separately

The residual plot draws its own SVG rather than going through a point renderer, so it is a fourth surface that draws a point — after the canvas renderer (#2696), PIXI (#2697) and the legend key (#2698). It stayed circles whatever the plot above it was drawn with. This is the last one.

The scope was wider than the code claimed

A comment in use-residual-plot.ts said residualPlotIsApplicable excludes legends, so the per-case legend lookup was dead. That is not what it does: it turns the plot off for a categorical legend only when the line is an LSRL. With a movable line or a plotted function a categorical legend is allowed, so the legend path is live — and per-category shapes genuinely reach these points, rather than only the display's own shape. The comment is corrected.

Rendering

A point is a <path> positioned with translate(cx, cy). The radius moves to a data-r attribute, because an outline has no radius to read back the way a circle does, and the existing fade-in machinery probes it.

The grow animation regenerates the outline rather than scaling the point. A scale() transform takes the stroke with it, and the stroke is what marks a point as selected — so a selected point would have faded in with an outline that thickened as it grew. Regenerating via attrTween leaves the whole existing fade-in intact: the named "radius" transition, the active() guard, and the deliberate choice not to redirect a transition already past its starting phase.

The shape comes from the same getLegendShapeForCase lookup the main plot uses, so the two plots agree by construction.

Testing

Seven Cypress assertions counted circle elements and now count paths. The marquee is untouched — it indexes positions it computes itself rather than querying elements.

No unit tests are added. use-residual-plot.ts has none and this change does not create a testable seam: it is d3 rendering inside a hook with layout and context dependencies, and the pure functions it leans on (pointShapePathData, residualPointStyle) are already covered. Verified manually instead — including a point fading in and a selected point fading in, which is the case that distinguishes this approach from scaling.

🤖 Generated with Claude Code

@kswenson kswenson added the v3 CODAP v3 label Sep 10, 2026
@kswenson

Copy link
Copy Markdown
Member Author

Reviewing this stack — seven 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
#2697 CODAP-1509 PIXI/WebGL rendering and hit testing
#2698 CODAP-1507 Legend keys
#2699 CODAP-1526 Residual plot — this one

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 five map one-to-one.

Nothing here is superseded — this is the top of the stack, and the last of the four surfaces that draw a point.

Worth your attention:

  • The fade-in. A new residual point grows from nothing, and this regenerates its outline at each step rather than scaling it — because a scale would take the stroke with it, and the stroke is how a selected point is marked. That is the part most worth a skeptical look, and the one thing here with no unit coverage.
  • The corrected comment. The file claimed residual plots never have a legend, which is why the per-case lookup looked dead. It is only excluded for a categorical legend with an LSRL, so a movable line or plotted function has one and per-category shapes do reach these points.
  • point-shapes.ts is unchanged from CODAP-1507: draw legend keys as the category's point shape #2698. Anything about the geometry belongs there.

This completes the set: canvas (#2696), PIXI (#2697), legend keys (#2698), residual plot (#2699). Nothing else in the codebase draws a per-case mark.

@codecov

codecov Bot commented Sep 10, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 83.33333% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.93%. Comparing base (e6ae88f) to head (c66c838).

Files with missing lines Patch % Lines
...ents/graph/plots/scatter-plot/use-residual-plot.ts 83.33% 3 Missing ⚠️
Additional details and impacted files
@@                       Coverage Diff                        @@
##           CODAP-1507-legend-key-shapes    #2699      +/-   ##
================================================================
- Coverage                         87.94%   87.93%   -0.02%     
================================================================
  Files                               821      821              
  Lines                             47566    47567       +1     
  Branches                          12142    12143       +1     
================================================================
- Hits                              41834    41826       -8     
- Misses                             5716     5725       +9     
  Partials                             16       16              
Flag Coverage Δ
cypress 69.59% <81.25%> (-0.03%) ⬇️
jest 63.25% <11.11%> (+<0.01%) ⬆️

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.

@kswenson
kswenson requested a balanced review from Copilot September 10, 2026 15:04
@cypress

cypress Bot commented Sep 10, 2026

Copy link
Copy Markdown

codap-v3    Run #12248

Run Properties:  status check passed Passed #12248  •  git commit c66c838539: null
Project codap-v3
Branch Review CODAP-1526-residual-plot-shapes
Run status status check passed Passed #12248
Run duration 09m 04s
Commit git commit c66c838539: null
Committer null
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 2
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.

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.


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

@kswenson
kswenson force-pushed the CODAP-1507-legend-key-shapes branch from 6c717ef to c3eec55 Compare September 10, 2026 15:48
@kswenson
kswenson force-pushed the CODAP-1526-residual-plot-shapes branch from 03a3ae8 to fd5e6a3 Compare September 10, 2026 15:49
@kswenson
kswenson marked this pull request as ready for review September 10, 2026 16:02
@kswenson
kswenson force-pushed the CODAP-1507-legend-key-shapes branch from c3eec55 to e6ae88f Compare September 11, 2026 03:14
The residual plot draws its own SVG rather than going through a point renderer,
so its points stayed circles whatever the upper plot was drawn with. It is the
fourth and last surface that draws a point, after the two renderers and the
legend key.

A point is an outline now, positioned by a translate rather than a center. The
radius moves to a data attribute because an outline has none to read back, and
everything that decides how a point is drawn reads it from there.

Growing a new point regenerates its outline at each step rather than scaling it.
A scale takes the stroke with it, and the stroke is what marks a point selected,
so a selected point would have faded in with an outline that thickened as it
grew. Regenerating leaves the whole of the existing fade-in alone, including the
guard against redirecting a transition that is already running.

The shape comes from the same lookup the upper plot uses, so the two agree.

Corrects the claim that a residual plot never has a legend. It is turned off for
a categorical legend only when the line is an LSRL, so a movable line or a
plotted function has one -- which is what makes a shape per category reach these
points at all, rather than only the display's own.

Seven Cypress assertions counted circles. The marquee is untouched: it indexes
positions it computes itself, not the elements.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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