feat(donut-s2): add AdvancedLabel (swatch + name/value/detail block) - #901
feat(donut-s2): add AdvancedLabel (swatch + name/value/detail block)#901madelineluke wants to merge 4 commits into
Conversation
|
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 |
| * @param donutOptions | ||
| * @returns Signal[] | ||
| */ | ||
| export const getAdvancedLabelSignals = (donutOptions: DonutSpecOptions): Signal[] => { |
There was a problem hiding this comment.
Why did we decide to base size scaling on diameter rather than chart width?
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>
7e50417 to
cec9eef
Compare
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>
|
|
🎨 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/ |
2 participants
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