-
Notifications
You must be signed in to change notification settings - Fork 2.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Badge] Fix font sizes, padding, and colors to align with visual design #23379
Conversation
📊 Bundle size report
Unchanged fixtures
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 7297dec:
|
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: d1f223054dba804964e010518ec0d09dbca2c584 (build) |
Perf Analysis (
|
Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
---|---|---|---|---|---|
Avatar | mount | 1279 | 1258 | 5000 | |
Button | mount | 886 | 883 | 5000 | |
FluentProvider | mount | 1568 | 1603 | 5000 | |
FluentProviderWithTheme | mount | 552 | 551 | 10 | |
FluentProviderWithTheme | virtual-rerender | 535 | 536 | 10 | |
FluentProviderWithTheme | virtual-rerender-with-unmount | 571 | 577 | 10 | |
MakeStyles | mount | 1957 | 1961 | 50000 | |
SpinButton | mount | 2430 | 2449 | 5000 |
groupSet: { | ||
display: 'inline-flex', | ||
flexDirection: 'column', | ||
...shorthands.padding(0, '16px'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
super nit: Use spacing tokens for paddings, gaps, etc.
...shorthands.padding('2px'), | ||
...shorthands.gap('4px'), | ||
fontSize: '8px', | ||
...shorthands.padding(0, '4px'), // 2px root padding + 2px text padding |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My first thought is that we should be using spacing tokens here but the comment is making me think you intentionally didn't use them.
Is there a reason to not use spacing tokens here? If so, can the comment explicitly state that so we don't update them at a later date? If not, can they be changed to tokens?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call. It's listed in the spec as pixel values, but that might have been written before there were spacing tokens. I'll follow up with design to double check if these should be using tokens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to use tokens in this PR. I'm still waiting to hear back from design, but it does seem like the right approach.
packages/react-components/react-badge/src/components/Badge/useBadgeStyles.ts
Outdated
Show resolved
Hide resolved
packages/react-components/react-badge/src/components/Badge/useBadgeStyles.ts
Outdated
Show resolved
Hide resolved
packages/react-components/react-badge/src/components/Badge/useBadgeStyles.ts
Outdated
Show resolved
Hide resolved
packages/react-components/react-badge/src/components/Badge/useBadgeStyles.ts
Outdated
Show resolved
Hide resolved
packages/react-components/react-badge/src/components/Badge/useBadgeStyles.ts
Outdated
Show resolved
Hide resolved
packages/react-components/react-badge/src/components/Badge/useBadgeStyles.ts
Show resolved
Hide resolved
packages/react-components/react-badge/src/components/Badge/useBadgeStyles.ts
Show resolved
Hide resolved
}, | ||
small: { | ||
minWidth: '16px', | ||
height: '16px', | ||
...shorthands.padding('2px'), | ||
...shorthands.gap('4px'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our current browser support matrix actually supports flex gap
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we're still determining if we need to accommodate browsers that don't support flex gap.
In any case, this still needed to be fixed to accommodate the extra padding that needed to be added around the text but not the icon, and the resulting styles wouldn't be made much simpler by using flex gap.
Also, the gap here would have been applied to any child elements as well (if the badge contained elements other than just text), which is probably unexpected.
…gn (microsoft#23379) Update Badge font sizes, padding, and colors to match the design. Add screener stories for Badge's size variations.
Current Behavior
New Behavior
:after
pseudo-element to avoid affecting layout between ghost and other appearancesRelated Issue(s)