feat: s2 donut ring width and slice gap - #891
Open
madelineluke wants to merge 4 commits into
Open
Conversation
10 tasks
madelineluke
marked this pull request as draft
August 19, 2026 22:05
madelineluke
changed the title
madelineluke
force-pushed
the
fix/donut-ring-width-per-tier
branch
from
August 27, 2026 20:23
adaa194 to
aa6f478
Compare
madelineluke
changed the base branch from
main
to
feat/donut-summary-responsive-sizing
August 27, 2026 20:23
Confirmed by pixel-measuring the Figma "Donut sizes" reference: the actual ring width per named size tier (S/M/L) matches the documented chart.donut.size.ring-width tokens almost exactly, while the current holeRatio=0.85-based innerRadius calculation renders a ring roughly half as thick at S, worsening at larger tiers. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… of holeRatio getArcMark/getEmptyStateArcMark computed innerRadius as a fixed proportion of the outer radius (holeRatio * DONUT_RADIUS), but the S2 design system specifies a near-fixed ring width per named size tier (16/18/20/22/24px for XS/S/M/L/XL) that grows far more slowly than diameter. Confirmed by pixel-measuring the Figma reference: the actual ring was roughly half as thick as intended at the S tier, worsening at larger sizes. Ring width now snaps to the nearest named tier (matching the pattern already used for DonutSummary font sizing) whenever holeRatio is left at its default - an explicitly customized holeRatio is still honored as a proportional ring. getArcMark, getEmptyStateArcMark, and DonutSummary's truncation/fit-gate formulas all share one getDonutInnerRadiusExpr() helper so they can't drift out of sync again. Discovered mid-fix: the corrected (thicker) ring at the S tier drops its inner radius below the old DONUT_SUMMARY_MIN_RADIUS fit threshold, which would have hidden center text that used to render - a new s2-only DONUT_SUMMARY_MIN_RADIUS_S2 keeps S-tier text visible without touching s1's shared constant. Also recalibrates the arc segment padAngle to a constant ~1px visual gap (1 / DONUT_RADIUS) regardless of donut size, replacing a fixed 0.01 radian value that was too thin at every size and confirmed against the Figma reference. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The previous commit's padAngle fix used a single constant-pixel-gap heuristic. The S2 donut chart tokens actually specify a per-named-tier slice gap (chart.donut.size.slice-gap: XS=1px/S=2px/M=2px/L=2px/XL=4px) - the same discrete tier-lookup shape already used for ring width and DonutSummary font sizing, not a single flat value. Adds getSliceGapScale/getSliceGapSignal (mirroring getRingWidthScale/getRingWidthSignal) and converts the resolved px gap to an angle at the outer radius for the arc mark's padAngle. Unlike ring width, slice gap doesn't depend on holeRatio, so it's registered unconditionally for every donut. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…egments A donut with many segments of highly skewed sizes (e.g. sliveredDonutData's 15 browsers, from Chrome's 34K down to SlimBrowser's 120) could have its smallest segments' entire angular width consumed by the fixed per-tier slice gap, rendering them as degenerate zero-area paths instead of a smaller gap. Cap padAngle to a fraction of each segment's own arcLength (already computed by the pie transform) rather than an average across all segments - an average gets dragged up by the larger segments and wouldn't meaningfully shrink the gap for the specific tiny ones actually at risk. Verified in Storybook: all 15 arcs in the Slivers story now render with proper (non-degenerate) SVG arc commands. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
madelineluke
force-pushed
the
fix/donut-ring-width-per-tier
branch
from
August 27, 2026 20:55
aa6f478 to
3e382ba
Compare
|
|
🎨 Storybook -> https://opensource.adobe.com/react-spectrum-charts/PR-891 🎨 S2 Storybook -> https://opensource.adobe.com/react-spectrum-charts/PR-891-s2 📚 Docs -> https://opensource.adobe.com/react-spectrum-charts/PR-891-docs/ |
madelineluke
marked this pull request as ready for review
August 27, 2026 21:14
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
Fixes
getArcMark/getEmptyStateArcMark's ring geometry:innerRadiuswas computed asholeRatio * DONUT_RADIUS(a fixed proportion of the outer radius), but the S2 design system specifies a near-fixed ring width per named size tier (16/18/20/22/24px for XS/S/M/L/XL) that grows far more slowly than diameter. Confirmed by pixel-measuring the Figma reference: the ring was roughly half as thick as intended at the S tier, worsening at larger sizes.Ring width now snaps to the nearest named tier whenever
holeRatiois left at its default (DEFAULT_HOLE_RATIO); an explicitly customizedholeRatiois still honored as a proportional ring, preserving backward compatibility.getArcMark,getEmptyStateArcMark, andDonutSummary's truncation/fit-gate formulas all share onegetDonutInnerRadiusExpr()helper so they can't drift out of sync again.Discovered mid-fix: the corrected (thicker) ring at the S tier drops its inner radius below the old
DONUT_SUMMARY_MIN_RADIUSfit threshold, which would have hidden center text that used to render. Introduces a new s2-onlyDONUT_SUMMARY_MIN_RADIUS_S2to keep S-tier text visible, without touching s1's shared constant.Also fixes the arc segment
padAngle: previously a flat0.01(too thin at every size). Now converts the per-tierchart.donut.size.slice-gaptoken (1/2/2/2/4px for XS/S/M/L/XL, perllm/skills/s2-donut/donut-chart-tokens/SKILL.md) to an angle at the outer radius, via the same tier-snapping pattern as ring width.Related Issue
Fixes
planning/specs/donut/issues/implemented/donut-ring-width-holeratio-mismatch.json. No GitHub issue tracked.Motivation and Context
The donut's rendered ring didn't match the Figma design at any size tier, and the gap between adjacent segments was too thin and didn't scale per tier as the design specifies.
How Has This Been Tested?
yarn test --testPathPattern="react-spectrum-charts-s2|vega-spec-builder-s2"- 2122/2122 passingyarn lint- cleanyarn tsc --noEmit- no new errors (pre-existing unrelated errors confirmed present onmain)Donut/Features/Donut Summary/Size Tiers) that ring width and segment gaps scale correctly per tierScreenshots (if appropriate):
N/A - verified in Storybook, see test plan above.
Types of changes
Checklist: