feat(donut-s2): add AdvancedLabel (swatch + name/value/detail block) - #901
Open
madelineluke wants to merge 5 commits into
Open
feat(donut-s2): add AdvancedLabel (swatch + name/value/detail block)#901madelineluke wants to merge 5 commits into
madelineluke wants to merge 5 commits into
Conversation
Collaborator
Author
|
Follow-up from live testing: confirmed two real layout issues not caught by unit tests:
Tried a horizontal fix mirroring the left hemisphere's capped pull-back - it kept content on-canvas, but did so by shrinking the fixed 20px ring-gap on oversized right-hemisphere labels, which isn't acceptable. Reverted. Decision: defer both issues to 🤖 Generated with Claude Code |
c-lamoureux
reviewed
Sep 1, 2026
madelineluke
force-pushed
the
feat/donut-advanced-labels
branch
from
September 3, 2026 16:15
7e50417 to
cec9eef
Compare
|
🎨 Storybook -> https://opensource.adobe.com/react-spectrum-charts/PR-901 🎨 S2 Storybook -> https://opensource.adobe.com/react-spectrum-charts/PR-901-s2 📚 Docs -> https://opensource.adobe.com/react-spectrum-charts/PR-901-docs/ |
Adds the AdvancedLabel Donut child: a color swatch + segment name row, optional bold value/percent row, and optional "X out of Y" detail row, positioned around the ring using the same hemisphere-mirrored anchor technique as SegmentLabel but extended to a 4-part block. Reuses the existing ring-gap/horizontal-reach reservation ratio to also cap the block's vertical row-stacking height, so a segment anchored near the ring's top/bottom never overflows without shrinking the ring further. Collision avoidance between adjacent label blocks (donut-direct-labels- collision) was explicitly deferred and is not integrated here - tracked as a known limitation in the reconciled spec. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…defer to collision Live testing surfaced two real issues not caught by unit tests: a right-hemisphere label block wider than the reserved margin overflows the container edge outright (no horizontal protection existed), and the existing vertical row-stacking cap compresses row spacing at small sizes as a side effect. A horizontal fix was attempted but reverted - it kept content on-canvas by shrinking the fixed 20px ring-gap, which isn't acceptable. Both issues are deferred to donut-direct-labels- collision as a single unit of work, since the real fix likely needs a continuous font-size shrink factor rather than position/spacing tricks. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Advanced-label name/value/detail sizes were one tier too large, and the segment-name row's "same size as Direct label - value" aliasing was wrong. Give segment name its own dedicated per-tier array (DONUT_ADVANCED_LABEL_NAME_FONT_SIZES) and shift all three arrays (name/value/detail) down one tier, deriving XS by extending the corrected S->M step downward - same correction methodology used for Direct labels. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… code smells in advancedLabelUtils Extracts getAdvancedLabelRowDy's nested ternary/template-literal constructs (totalHeightExpr, bottomDetailDy, topValueDy, topNameDy) into named consts and if/else chains, and switches the percent+value signal concatenation to String.raw to avoid the escaped backslash. No behavior change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Not present in the Figma spec and not needed. value/percent were
almost always OR'd together (hasValue = value || percent), so most
sites simplify to just percent. The one real behavior change is
getAdvancedLabelValueText, which previously supported a combined
"percent + raw value" display (e.g. "60.8% 6,458") - that combined
mode is removed along with the standalone value-only mode, leaving
percent-only. valueFormat is kept, re-scoped to the detail row's
"{value} out of {total}" number formatting, which is unrelated to the
removed value boolean.
Updates isInteractive() (markUtils.ts), the AdvancedLabel component
default, stories, and tests accordingly, and corrects the two
now-inaccurate prop-signature references in the implemented feature
spec.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
madelineluke
force-pushed
the
feat/donut-advanced-labels
branch
from
September 8, 2026 21:49
e5615bc to
5882d3c
Compare
|
madelineluke
removed this pull request from stack #898
September 9, 2026 22:41
madelineluke
added this pull request to stack #908
September 9, 2026 22:42
madelineluke
removed this pull request from stack #908
September 9, 2026 22:42
madelineluke
added this pull request to stack #909
September 9, 2026 22:43
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
2 participants
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.
Summary
AdvancedLabelDonut child component - a color swatch + segment name row, optional bold value/percent row, and optional "X out of Y" detail row, positioned around the ring, perplanning/specs/donut/implemented/donut-advanced-labels.json.SegmentLabel) to a 4-part swatch+text block, including manually replicating Vega's polar-to-cartesian math for the swatch since symbol marks have noradius/theta/dxencode channels.AdvancedLabelis present (previously onlySegmentLabeltriggered this), and caps the block's vertical row-stacking height using the same existing horizontal-reach ratio, so a segment anchored near the ring's top/bottom never overflows without shrinking the ring further than direct labels already do.getSegmentLabelValueFillinto a sharedgetLabelValueFill(donutOptions, restColor)so both direct and advanced labels reuse the same hover-color-switch logic, and extendsisInteractive()to treat a value/percent-showingAdvancedLabelas interactive.sanitizeChildrenallowlist bug found during implementation: it maintains its own independentvalidDisplayNamesset separate fromchildrenAdapter.ts's switch, which was silently droppingAdvancedLabelchildren before they ever reached the adapter.donut-direct-labels-collision) was explicitly deferred and is not integrated in this PR - documented as a known limitation in the reconciled spec.Test plan
yarn test- 4224 passed (41 new inadvancedLabelUtils.test.tsx, plus new coverage inmarkUtils.test.tsanddonutSpecBuilder.test.ts)yarn tsc --noEmit- no new errorsyarn lint- cleanBasic,NameOnly, and a new responsiveWithDetailstory (width-slider, mirroringDirectLabels.story.tsx) spanning all size tiers (60px-674px) - confirmed the vertical-overflow fix holds (no label escapes its reserved margin) and the ring stays full-size; XS/S-tier adjacent-label overlap is a known, separately-tracked limitation (no collision avoidance yet)🤖 Generated with Claude Code