Conversation
✅ Heimdall Review Status
✅
|
| Code Owner | Status | Calculation | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| ui-systems-eng-team |
✅
1/1
|
Denominator calculation
|
| tabs={tabs} | ||
| width="100%" | ||
| /> | ||
| ); |
There was a problem hiding this comment.
Fixed a bug where it would throw an error if you selected a tab
| | 'compact' | ||
| | 'disabled' | ||
| | 'uncheckedLabel' | ||
| | 'variant' |
There was a problem hiding this comment.
I wonder if we should create a shared type for al border props (and maybe subtype for border radius props.
There was a problem hiding this comment.
Maybe not necessary at this point. Unless we find out we are explicitly using border props everywhere in CDS components. In that case we can do a non-breaking refactor later.
| activeBackground = 'bgInverse', | ||
| background = 'bgSecondary', | ||
| borderRadius = 1000, | ||
| borderRadius = 700, |
There was a problem hiding this comment.
Small change to technically match design, shouldn't have a visual impact unless someone had larger spacing tokens.
| tab: 'cds-PeriodSelector-tab', | ||
| /** Active indicator element */ | ||
| activeIndicator: 'cds-PeriodSelector-activeIndicator', | ||
| } as const; |
There was a problem hiding this comment.
Thoughts on this? Customers get these static classnames at each level, meaning you have all three as classNames.
cds-Tabs-tab cds-SegmentedTabs-tab cds-PeriodSelector-tab
I see the benefit, so someone could target. But also wondering if we should allow an overwrite, and just have cds-PeriodSelector-tab.
There was a problem hiding this comment.
I dropped static classnames for now, per discussion in Slack
| /** Active indicator element */ | ||
| activeIndicator?: StyleProp<ViewStyle>; | ||
| }; | ||
| }; |
There was a problem hiding this comment.
With TabsProps being generic, I wasn't able to get the comments from it to show in docs, so recreating I thought is best best workaround until we decide static classnames here.
…inbase/cds into hunter/tabs-styles-classnames
| activeBackground = 'bgInverse', | ||
| background = 'bgSecondary', | ||
| borderRadius = 1000, | ||
| borderRadius = 700, |
There was a problem hiding this comment.
To match design we updated it, again shouldn't have issues. For PeriodSelector design shows 1000.
| | 'compact' | ||
| | 'disabled' | ||
| | 'uncheckedLabel' | ||
| | 'variant' |
There was a problem hiding this comment.
Maybe not necessary at this point. Unless we find out we are explicitly using border props everywhere in CDS components. In that case we can do a non-breaking refactor later.
| {...props} | ||
| > | ||
| <Box as="span" justifyContent="center" paddingX={2} paddingY={1}> | ||
| <MotionBox as="span" justifyContent="center" paddingX={2} paddingY={1} {...motionProps}> |
There was a problem hiding this comment.
What's the reason for moving motion from text to the wrapping box, and do we have any regression risk from this change?
3 participants
What changed? Why?
This PR
Root cause (required for bugfixes)
UI changes
Docs
Components
Testing
How has it been tested?
Testing instructions
Illustrations/Icons Checklist
Required if this PR changes files under
packages/illustrations/**orpackages/icons/**Change management
type=routine
risk=low
impact=sev5
automerge=false