-
Notifications
You must be signed in to change notification settings - Fork 55
[CAP-Theme] add Label component styles and integrate into CAP theme #579
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
base: main
Are you sure you want to change the base?
Changes from all commits
c2f6df1
5f1f03e
bdc2184
f4483c3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| { | ||
| "type": "minor", | ||
| "comment": "Generated files for Label component", | ||
| "packageName": "@fluentui-contrib/react-cap-theme", | ||
| "email": "olkatruk@microsoft.com", | ||
| "dependentChangeType": "patch" | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| import { | ||
| makeStyles, | ||
| mergeClasses, | ||
| tokens, | ||
| type LabelState, | ||
| } from '@fluentui/react-components'; | ||
|
|
||
| export const useLabelStyles = makeStyles({ | ||
| root: { | ||
| display: 'inline-flex', | ||
| alignItems: 'center', | ||
| gap: tokens.spacingHorizontalXS, | ||
| }, | ||
| startSlot: { | ||
| marginRight: tokens.spacingHorizontalXXS, | ||
| }, | ||
| required: { | ||
| paddingLeft: 0, | ||
| }, | ||
| }); | ||
|
|
||
| export function useLabelStylesHook(state: LabelState): LabelState { | ||
| const styles = useLabelStyles(); | ||
|
|
||
| state.root.className = mergeClasses(state.root.className, styles.root); | ||
| if (state.required) { | ||
| state.required.className = mergeClasses( | ||
| state.required.className, | ||
| styles.required | ||
| ); | ||
| } | ||
|
|
||
| return state; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| import * as React from 'react'; | ||
| import type { ForwardRefComponent } from '@fluentui/react-utilities'; | ||
| import { | ||
| Badge, | ||
| Label as FluentLabel, | ||
| mergeClasses, | ||
| } from '@fluentui/react-components'; | ||
| import { CircleRegular } from '@fluentui/react-icons'; | ||
| import { useLabelStyles } from './Label.styles'; | ||
| import type { LabelProps } from './Label.types'; | ||
|
|
||
| const defaultStart = ( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| <Badge | ||
| appearance="ghost" | ||
| shape="rounded" | ||
| size="medium" | ||
| icon={<CircleRegular />} | ||
| /> | ||
| ); | ||
|
|
||
| export const Label = React.forwardRef<HTMLLabelElement, LabelProps>( | ||
| ({ badge, showBadge = false, children, className, ...rest }, ref) => { | ||
pixel-perfectionist marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| const styles = useLabelStyles(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Our components should be seen purely as overrides to existing styles, so we need to make sure to not silently drop existing styles. |
||
| const badgeContent = badge ?? (showBadge ? defaultStart : null); | ||
|
|
||
| return ( | ||
| <FluentLabel | ||
| {...rest} | ||
| ref={ref} | ||
| className={mergeClasses(styles.root, className)} | ||
| > | ||
| {badgeContent && ( | ||
| <span className={styles.startSlot}>{badgeContent}</span> | ||
| )} | ||
| {children} | ||
| </FluentLabel> | ||
| ); | ||
| } | ||
| ) as ForwardRefComponent<LabelProps>; | ||
|
|
||
| Label.displayName = 'Label'; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| import * as React from 'react'; | ||
| import type { | ||
| LabelProps as FluentLabelProps, | ||
| LabelState as FluentLabelState, | ||
| } from '@fluentui/react-components'; | ||
|
|
||
| export type LabelProps = FluentLabelProps & { | ||
| /** | ||
| * Optional leading badge rendered before the label text. | ||
| */ | ||
| badge?: React.ReactNode; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you please document that we had to add this prop in the Component Notes?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. with the current setup, should this be a Badge component type instead of |
||
| /** | ||
| * Controls whether the badge is shown when `badge` is not provided. | ||
| * @default false | ||
| */ | ||
| showBadge?: boolean; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe rename to showDefaultDotBadge to reduce confusion? also modify the comment accordingly
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar comment here: #579 (comment)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's confusing that passing A different idea would be to change |
||
| }; | ||
|
|
||
| export type LabelState = FluentLabelState; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,134 @@ | ||
| import * as React from 'react'; | ||
| import { Label, type LabelProps } from '@fluentui-contrib/react-cap-theme'; | ||
| import { Badge, type BadgeProps } from '@fluentui/react-components'; | ||
| import { CircleRegular } from '@fluentui/react-icons'; | ||
| import { CAPThemeExamplesTable } from '../../StorybookUtils'; | ||
|
|
||
| const DEFAULT_BADGE: Required< | ||
| Pick<BadgeProps, 'appearance' | 'shape' | 'size' | 'color'> | ||
| > = { | ||
| appearance: 'filled', | ||
| shape: 'rounded', | ||
| size: 'small', | ||
| color: 'informative', | ||
| }; | ||
|
|
||
| type LabelStoryProps = Omit<LabelProps, 'badge'> & { | ||
| badgeVisible?: boolean; | ||
| badgeAppearance?: BadgeProps['appearance']; | ||
| badgeShape?: BadgeProps['shape']; | ||
| badgeSize?: BadgeProps['size']; | ||
| badgeColor?: BadgeProps['color']; | ||
| badgeIcon?: boolean; | ||
| }; | ||
|
|
||
| export const CAPLabelStory = ({ | ||
| badgeVisible = true, | ||
| badgeAppearance = DEFAULT_BADGE.appearance, | ||
| badgeShape = DEFAULT_BADGE.shape, | ||
| badgeSize = DEFAULT_BADGE.size, | ||
| badgeColor = DEFAULT_BADGE.color, | ||
| badgeIcon = false, | ||
| ...props | ||
| }: LabelStoryProps) => { | ||
| const getBadge = () => | ||
| badgeVisible ? ( | ||
| <Badge | ||
| appearance={badgeAppearance} | ||
| shape={badgeShape} | ||
| size={badgeSize} | ||
| color={badgeColor} | ||
| icon={<CircleRegular />} | ||
| ></Badge> | ||
| ) : undefined; | ||
|
|
||
| return ( | ||
| <CAPThemeExamplesTable | ||
| examples={[ | ||
| { | ||
| title: 'Default', | ||
| render() { | ||
| return ( | ||
| <Label {...props} badge={getBadge()}> | ||
| Label | ||
| </Label> | ||
| ); | ||
| }, | ||
| }, | ||
| { | ||
| title: 'Required', | ||
| render() { | ||
| return ( | ||
| <Label required {...props} badge={getBadge()}> | ||
| Required Label | ||
| </Label> | ||
| ); | ||
| }, | ||
| }, | ||
| { | ||
| title: 'Disabled', | ||
| render() { | ||
| return ( | ||
| <Label disabled {...props} badge={getBadge()}> | ||
| Disabled Label | ||
| </Label> | ||
| ); | ||
| }, | ||
| }, | ||
| ]} | ||
| /> | ||
| ); | ||
| }; | ||
|
|
||
| CAPLabelStory.argTypes = { | ||
| size: { | ||
| options: ['small', 'medium', 'large'], | ||
| control: { type: 'radio' }, | ||
| }, | ||
| weight: { | ||
| options: ['regular', 'semibold'], | ||
| control: { type: 'radio' }, | ||
| }, | ||
| badgeVisible: { | ||
| control: { type: 'boolean' }, | ||
| }, | ||
| badgeAppearance: { | ||
| options: ['filled', 'ghost', 'outline', 'tint'], | ||
| control: { type: 'radio' }, | ||
| }, | ||
| badgeShape: { | ||
| options: ['rounded', 'circular', 'square'], | ||
| control: { type: 'radio' }, | ||
| }, | ||
| badgeSize: { | ||
| options: ['tiny', 'extra-small', 'small', 'medium', 'large', 'extra-large'], | ||
| control: { type: 'radio' }, | ||
| }, | ||
| badgeColor: { | ||
| options: [ | ||
| 'brand', | ||
| 'danger', | ||
| 'important', | ||
| 'informative', | ||
| 'severe', | ||
| 'subtle', | ||
| 'success', | ||
| 'warning', | ||
| ], | ||
| control: { type: 'radio' }, | ||
| }, | ||
| badgeIcon: { | ||
| control: { type: 'boolean' }, | ||
| }, | ||
| }; | ||
|
|
||
| CAPLabelStory.args = { | ||
| size: 'medium', | ||
| weight: 'regular', | ||
| badgeVisible: true, | ||
| badgeAppearance: DEFAULT_BADGE.appearance, | ||
| badgeShape: DEFAULT_BADGE.shape, | ||
| badgeSize: DEFAULT_BADGE.size, | ||
| badgeColor: DEFAULT_BADGE.color, | ||
| badgeIcon: false, | ||
| }; |
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.
Should we be introducing new tokens here so that this follows the same convention as the other components?