[CAP-Theme] add Label component styles and integrate into CAP theme #579
Conversation
pixel-perfectionist
force-pushed
the
olkatruk/cap-theme/label
branch
3 times, most recently
from
85c506a to
5e993c6
Compare
pixel-perfectionist
changed the title
pixel-perfectionist
changed the title
pixel-perfectionist
force-pushed
the
olkatruk/cap-theme/label
branch
from
5e993c6 to
f4483c3
Compare
sxsx1xsxs
left a comment
sxsx1xsxs
left a comment
There was a problem hiding this comment.
-
I see in figma, for small size, the badge should also change size https://www.figma.com/design/40dstXfCx3jpvlP4prbSBQ/Semantic-Tokens-mapping?node-id=6238-50140&m=dev
-
I played around with the current setup, with "tiny" badge, the badge doesn't seem to be vertically centered with the label
| import { useLabelStyles } from './Label.styles'; | ||
| import type { LabelProps } from './Label.types'; | ||
|
|
||
| const defaultStart = ( |
There was a problem hiding this comment.
the size is not always medium, and I added a question to drieli if it's always gona be this icon or it's just a placeholder here https://www.figma.com/design/40dstXfCx3jpvlP4prbSBQ?node-id=6290-42144&m=dev#1548157118
There was a problem hiding this comment.
I understand you are saying we could pass in the badge component to Label component but also offering a default one, but I kinda think the size of the badge should not be indepedent of the label size. Like once the user specify "medium" label, we should configure the size of the badge like what figma indicates/or at least the default size would change according to the size of the label. Right now, in this setup, you could provide a extra large badge to a small label, which doesn't make sense.
| * Controls whether the badge is shown when `badge` is not provided. | ||
| * @default false | ||
| */ | ||
| showBadge?: boolean; |
There was a problem hiding this comment.
maybe rename to showDefaultDotBadge to reduce confusion? also modify the comment accordingly
| /** | ||
| * Optional leading badge rendered before the label text. | ||
| */ | ||
| badge?: React.ReactNode; |
There was a problem hiding this comment.
with the current setup, should this be a Badge component type instead of React.ReactNode?
| * Controls whether the badge is shown when `badge` is not provided. | ||
| * @default false | ||
| */ | ||
| showBadge?: boolean; |
There was a problem hiding this comment.
It's confusing that passing showBadge={false} will not cause the badge to be hidden.
A different idea would be to change badge?: React.ReactNode | true (which is functionally equivalent to badge?: React.ReactNode, just more explicit), where passing true causes it to render the default badge?
| /** | ||
| * Optional leading badge rendered before the label text. | ||
| */ | ||
| badge?: React.ReactNode; |
There was a problem hiding this comment.
Can you please document that we had to add this prop in the Component Notes?
|
|
||
| export const Label = React.forwardRef<HTMLLabelElement, LabelProps>( | ||
| ({ badge, showBadge = false, children, className, ...rest }, ref) => { | ||
| const styles = useLabelStyles(); |
There was a problem hiding this comment.
This is missing a few important elements from the upstream Label implementation: https://github.com/microsoft/fluentui/blob/master/packages/react-components/react-label/library/src/components/Label/Label.tsx#L15-L21.
- It needs to call
useLabelStyles_unstable(state);. - It also needs to call
useCustomStyleHook_unstable('useLabelStyles_unstable')(state);in order to pull in any custom styles, otherwise they will silently disappear when the core component is swapped for this one.
Our components should be seen purely as overrides to existing styles, so we need to make sure to not silently drop existing styles.
| root: { | ||
| display: 'inline-flex', | ||
| alignItems: 'center', | ||
| gap: tokens.spacingHorizontalXS, |
There was a problem hiding this comment.
Should we be introducing new tokens here so that this follows the same convention as the other components?
Enhance Label component with new styles and badge integration