feat(donut-s2): Direct Labels - base implementation, positioning and scaling - #894
Open
madelineluke wants to merge 4 commits into
Open
Conversation
madelineluke
marked this pull request as draft
August 26, 2026 21:49
madelineluke
changed the base branch from
main
to
feat/donut-summary-responsive-sizing
August 27, 2026 18:16
madelineluke
force-pushed
the
feat/donut-direct-labels-ring-gap-sizing-anchor
branch
from
August 27, 2026 20:23
38b9e40 to
b471c34
Compare
madelineluke
changed the base branch from
feat/donut-summary-responsive-sizing
to
fix/donut-ring-width-per-tier
August 27, 2026 20:23
…ize reactivity - Add getDonutOuterRadiusExpr to reserve room for the direct-label ring gap and hemisphere pull-back within the donut's own radius, so the ring and its labels deterministically fit any container size without relying on Vega's autosize growing beyond it. Threaded through the arc, empty-state ring, and DonutSummary radius/font-size calculations. - Move segment label radius/dx/fontSize/dy/align/baseline encodings from Vega's `enter` set (evaluated once at mark creation) into `update`, since they depend on width/height-derived signals that must recompute on resize. Previously labels only translated along with the container's center shift while the ring itself resized correctly, causing labels to visually detach from the ring at certain sizes. - Add a width-slider-driven Responsive story (matching Line's DirectLabelSizeScaling pattern) plus a ManySegmentsResponsive variant using the existing sliveredDonutData fixture, replacing the earlier CSS-resize-based story. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ability The story computed size tiers from raw container width, but the real spec keys tiers off the label-reserved outer diameter (getDonutOuterRadiusExpr), so the displayed "Size tier" text and threshold-line positions didn't match what actually rendered. Also fixes XL being structurally unreachable because the fixed chart height capped min(width, height) before the width slider could reach it.
madelineluke
force-pushed
the
feat/donut-direct-labels-ring-gap-sizing-anchor
branch
from
August 27, 2026 20:55
b471c34 to
33d3b7c
Compare
…der, segmentLabelUtils Replace length-1 indexing with .at(-1) (guarded against undefined, matching the existing pattern in barUtils.ts), consolidate consecutive Array#push() calls, and guard getTextRuleExpr's field access against Field's non-string variants to avoid stringifying an object as "[object Object]".
Export getTextRuleExpr and add direct unit tests for its field, value, and empty-rule fallback branches, plus the empty-array guard - none of these were reachable through getSegmentLabelValueText, the only real caller, which always produces signal-shaped rules.
|
|
🎨 Storybook -> https://opensource.adobe.com/react-spectrum-charts/PR-894 🎨 S2 Storybook -> https://opensource.adobe.com/react-spectrum-charts/PR-894-s2 📚 Docs -> https://opensource.adobe.com/react-spectrum-charts/PR-894-docs/ |
madelineluke
marked this pull request as ready for review
August 27, 2026 21:53
madelineluke
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Implements the ring-gap sizing/hemisphere-anchor portion of the
donut-direct-labelsspec, and fixes a real resize-reactivity bug found while building the responsive Storybook example for it.getDonutOuterRadiusExprto deterministically reserve room for the direct-label ring gap and hemisphere pull-back within the donut's own radius, so the ring and its labels always fit within a given container size in a single pass, without relying on Vega'sautosizegrowing beyond it. This is threaded through the arc mark, the empty-state ring, andDonutSummary's radius/font-size/truncation calculations so they all agree on the same effective radius.radius/dx/fontSize/dy/align/baselineon the segment label text marks lived in Vega'senterencode set, which only evaluates once when a mark is first created. On resize, only the sharedx/yanchor (inupdate) tracked the new container size, so labels were uniformly translated by the container's center shift instead of properly rescaling - causing them to visually detach from (or overlap into) the ring at certain sizes. Moved these encodings intoupdate.Responsivestory (matching the existingLineDirectLabelSizeScalingpattern, with visible size-tier breakpoint markers), plus aManySegmentsResponsivevariant using the existingsliveredDonutDatafixture to stress-test label crowding with 15 segments.Related Issue
planning/specs/donut/donut-direct-labels.json(approved)Motivation and Context
Direct labels on a donut need a ring gap and can pull back horizontally to avoid overlapping the ring near the 9/3 o'clock cardinal points. Without reserving space for that up front, labels could end up demanding more room than the container had, either forcing the chart to grow beyond its given size or causing the ring to shrink disproportionately. Fixing the resize-reactivity bug was necessary because the size-tier-driven font/anchor calculations only take effect on the very first render otherwise, making the ring and its labels visibly drift apart on any subsequent resize.
How Has This Been Tested?
segmentLabelUtils.test.tsxanddonutSummaryUtils.test.tsxfor the new radius-reservation formula and theenter→updateencode move.vega-spec-builder-s2/react-spectrum-charts-s2test suite passes (131 suites, 2125 tests).yarn lintandyarn tsc --noEmitare clean (no new errors; two pre-existing unrelated failures confirmed present on the base commit too).ResponsiveandManySegmentsResponsivestories: the ring and its labels now scale together in real time with no overlap, including immediately after a resize (previously frozen from the initial render).Screenshots (if appropriate):
See the
ResponsiveandManySegmentsResponsivestories underReact Spectrum Charts 2/Donut/Features/Direct Labelin Storybook.Types of changes
Checklist: