-
Notifications
You must be signed in to change notification settings - Fork 251
fix(fuselage): align Button with the design system
#2151
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
0a5c538
ce739b1
cdeccfb
53f047c
4477216
d6254b1
77316a9
3c83dd3
9b0b319
6b21fec
8ee18f5
2465928
5668c6a
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,5 @@ | ||
| --- | ||
| '@rocket.chat/fuselage': minor | ||
| --- | ||
|
|
||
| fix(fuselage): align `Button` with the design system |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| --- | ||
| '@rocket.chat/fuselage': patch | ||
| --- | ||
|
|
||
| Fix the disabled `secondary-danger` Button rendering the filled danger | ||
| background. `getPalette` mapped | ||
| `button-background-secondary-danger-disabled` to | ||
| `button.backgroundDangerDisabled` (`#FFC1C9`) instead of | ||
| `button.backgroundSecondaryDangerDisabled` (`#EBECEF`), so a disabled secondary | ||
| danger button appeared as a pink filled button rather than a greyed-out neutral | ||
| one. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -53,11 +53,12 @@ export default { | |
| control: 'select', | ||
| options: ['small', 'medium', 'large'], | ||
| description: 'Size scale of the button.', | ||
| table: { defaultValue: { summary: 'default (40px)' } }, | ||
| }, | ||
| square: { | ||
| control: 'boolean', | ||
| description: | ||
| 'Renders as a square icon-only footprint instead of the default pill shape.', | ||
| 'Renders the button as a square, label-less footprint sized to match neighbouring buttons and inputs.', | ||
| table: { category: 'Shape' }, | ||
| }, | ||
| icon: { | ||
|
|
@@ -309,3 +310,122 @@ export const States: Story = { | |
| </> | ||
| ), | ||
| }; | ||
|
|
||
| const SIZE_ROWS = [ | ||
| { label: 'small', size: 'small' as const }, | ||
| { label: 'medium', size: 'medium' as const }, | ||
| { label: 'default', size: undefined }, | ||
| { label: 'large', size: 'large' as const }, | ||
| ]; | ||
|
|
||
| /** | ||
| * Outlines the icon canvas and tints the gap that follows it, so both scale | ||
| * visibly with button size instead of having to be inferred from code. | ||
| */ | ||
| const canvasProbeStyles = ` | ||
| .rcx-icon-canvas-probe .rcx-button .rcx-icon { | ||
| outline: 1px dashed rgba(236, 13, 42, 0.9); | ||
| outline-offset: 0; | ||
| background: rgba(236, 13, 42, 0.18); | ||
| } | ||
| /* The label is a text node, so the gap is tinted by offsetting a shadow of | ||
| the icon box across the 4px margin that follows it. Scoped to | ||
| --with-icon, which is only set when a label actually follows the icon. */ | ||
| .rcx-icon-canvas-probe .rcx-button--with-icon .rcx-icon { | ||
| box-shadow: 4px 0 0 0 rgba(255, 255, 255, 0.55); | ||
| } | ||
| `; | ||
|
|
||
| export const IconAndLabel: Story = { | ||
| name: 'Icon and label', | ||
| parameters: { | ||
| docs: { | ||
| description: { | ||
| story: | ||
| 'How a leading icon pairs with a label at every size.\n\n' + | ||
| '**The rule.** The icon canvas is sized to the label’s line-height, so it ' + | ||
| 'scales with the button: 20px on the 40px and 48px buttons, 16px on the ' + | ||
| '32px and 28px ones. The gap between icon and label is a constant 4px and ' + | ||
| 'does **not** scale. The inset on the icon side is one 4px step tighter ' + | ||
| 'than the inset on the label side, so the icon reads as optically centred.\n\n' + | ||
| '**Reading the overlay.** The dashed red box is the icon canvas — note it ' + | ||
| 'is the canvas, not the glyph, which sits inside it. The pale band to its ' + | ||
| 'right is the 4px gap.', | ||
| }, | ||
| }, | ||
| }, | ||
| render: () => ( | ||
| <div className='rcx-icon-canvas-probe'> | ||
| <style>{canvasProbeStyles}</style> | ||
| <table style={{ borderCollapse: 'collapse' }}> | ||
| <thead> | ||
| <tr> | ||
| {['Size', 'Icon + label', 'Label only', 'Icon only'].map((h) => ( | ||
| <th | ||
| key={h} | ||
| style={{ | ||
| padding: '8px 16px', | ||
| textAlign: 'left', | ||
| font: '700 12px/16px Inter, sans-serif', | ||
| color: '#6C727A', | ||
| borderBottom: '1px solid #E4E7EA', | ||
| }} | ||
| > | ||
| {h} | ||
| </th> | ||
| ))} | ||
| </tr> | ||
| </thead> | ||
| <tbody> | ||
| {SIZE_ROWS.map(({ label, size }) => ( | ||
| <tr key={label}> | ||
| <td | ||
| style={{ | ||
| padding: '12px 16px', | ||
| font: '700 12px/16px Inter, sans-serif', | ||
| color: '#6C727A', | ||
| borderBottom: '1px solid #E4E7EA', | ||
| }} | ||
| > | ||
| {label} | ||
| </td> | ||
| <td | ||
| style={{ | ||
| padding: '12px 16px', | ||
| borderBottom: '1px solid #E4E7EA', | ||
| }} | ||
| > | ||
| <Button variant='primary' size={size} icon='baloon-text'> | ||
| Button | ||
| </Button> | ||
| </td> | ||
| <td | ||
| style={{ | ||
| padding: '12px 16px', | ||
| borderBottom: '1px solid #E4E7EA', | ||
| }} | ||
| > | ||
| <Button variant='primary' size={size}> | ||
| Button | ||
| </Button> | ||
| </td> | ||
| <td | ||
| style={{ | ||
| padding: '12px 16px', | ||
| borderBottom: '1px solid #E4E7EA', | ||
| }} | ||
| > | ||
| <Button | ||
| variant='primary' | ||
| size={size} | ||
| square | ||
| icon='baloon-text' | ||
| /> | ||
| </td> | ||
| </tr> | ||
| ))} | ||
| </tbody> | ||
| </table> | ||
|
Comment on lines
+360
to
+428
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. use |
||
| </div> | ||
| ), | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,15 +6,36 @@ | |
| @use '../../styles/mixins/interactivity.scss' as *; | ||
| @use '../../styles/mixins/states.scss' as *; | ||
|
|
||
| // Rendered inset = border + padding. Mixins below subtract this from the | ||
| // total inset to get the padding value. | ||
| $border-width: theme('button-border-width', lengths.border-width('default')); | ||
|
|
||
| // Inline insets, composed from the 4px padding scale | ||
| // (`lengths.padding()` accepts 1, 2, or a multiple of 4). | ||
| $inset-default: lengths.padding(16) - lengths.padding(2); // 14px | ||
| $inset-small: lengths.padding(8); // 8px | ||
| $inset-large: lengths.padding(24); // 24px | ||
|
|
||
| // Icon-side inset, one 4px step tighter than the label-side inset. | ||
| $inset-icon-default: lengths.padding(12); // 12px | ||
| $inset-icon-small: lengths.padding(8); // 8px | ||
| $inset-label-small: lengths.padding(8) + lengths.padding(2); // 10px | ||
| $inset-icon-large: lengths.padding(24) - lengths.padding(2); // 22px | ||
|
|
||
| .rcx-button { | ||
| @mixin with-rectangular-size($height, $padding-x, $line-height) { | ||
| @mixin with-rectangular-size($height, $inline-inset, $line-height) { | ||
| min-width: calc(lengths.size($height) * 2); | ||
| height: lengths.size($height); | ||
| padding: calc((lengths.padding($height) - $line-height) / 2 - 2px) | ||
| calc(lengths.padding($padding-x) - 2px); | ||
| padding-block: calc((lengths.padding($height) - $line-height) / 2 - 2px); | ||
|
|
||
| padding-inline: calc(lengths.padding($padding-x) - 2px); | ||
| padding-block: calc( | ||
| (lengths.size($height) - $line-height) / 2 - $border-width | ||
| ); | ||
| padding-inline: calc(#{$inline-inset} - #{$border-width}); | ||
|
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. no need for |
||
| } | ||
|
|
||
| @mixin with-leading-icon-inset($start, $end) { | ||
| padding-inline: calc(#{$start} - #{$border-width}) | ||
|
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. no need for |
||
| calc(#{$end} - #{$border-width}); | ||
| } | ||
|
|
||
| @mixin with-squared-size($size) { | ||
|
|
@@ -49,6 +70,12 @@ | |
| vertical-align: top; | ||
|
|
||
| @include use-with-truncated-text(); | ||
|
|
||
| // Icon is sized to the label's line-height, so top-aligning it keeps | ||
| // both centred (default `vertical-align: text-bottom` would not). | ||
| .rcx-icon { | ||
| vertical-align: top; | ||
| } | ||
| } | ||
|
|
||
| @include clickable; | ||
|
|
@@ -57,10 +84,17 @@ | |
|
|
||
| @include with-rectangular-size( | ||
| $height: 40, | ||
| $padding-x: 16, | ||
| $inline-inset: $inset-default, | ||
| $line-height: line-height(p2) | ||
| ); | ||
|
|
||
| &.rcx-button--with-icon { | ||
| @include with-leading-icon-inset( | ||
| $start: $inset-icon-default, | ||
| $end: $inset-default | ||
| ); | ||
| } | ||
|
|
||
| @include button.kind-variant(colors.$secondary); | ||
|
|
||
| &--loading { | ||
|
|
@@ -74,29 +108,50 @@ | |
|
|
||
| @include with-rectangular-size( | ||
| $height: 28, | ||
| $padding-x: 8, | ||
| $inline-inset: $inset-small, | ||
| $line-height: line-height(c1) | ||
| ); | ||
|
|
||
| &.rcx-button--with-icon { | ||
| @include with-leading-icon-inset( | ||
| $start: $inset-icon-small, | ||
| $end: $inset-label-small | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| &--medium { | ||
| @include use-font-scale(c2); | ||
|
|
||
| @include with-rectangular-size( | ||
| $height: 32, | ||
| $padding-x: 12, | ||
| $inline-inset: $inset-default, | ||
| $line-height: line-height(c1) | ||
| ); | ||
|
|
||
| &.rcx-button--with-icon { | ||
| @include with-leading-icon-inset( | ||
| $start: $inset-icon-default, | ||
| $end: $inset-default | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| &--large { | ||
| @include use-font-scale(p2); | ||
|
|
||
| @include with-rectangular-size( | ||
| $height: 48, | ||
| $padding-x: 24, | ||
| $inline-inset: $inset-large, | ||
| $line-height: line-height(p2) | ||
| ); | ||
|
|
||
| &.rcx-button--with-icon { | ||
| @include with-leading-icon-inset( | ||
| $start: $inset-icon-large, | ||
| $end: $inset-large | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| &--square { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -46,6 +46,28 @@ export type ButtonProps = Omit<BoxProps, 'ref'> & { | |
| > & | ||
| RefAttributes<HTMLButtonElement | HTMLAnchorElement>; | ||
|
|
||
| /** | ||
| * External links carry a `new-window` affordance unless the caller has already | ||
| * chosen a leading icon. | ||
| */ | ||
| const resolveIcon = ( | ||
| icon: ButtonProps['icon'], | ||
| is: ButtonProps['is'], | ||
| external: ButtonProps['external'], | ||
| ): IconProps['name'] | undefined => { | ||
| if (icon) { | ||
| return icon; | ||
| } | ||
|
|
||
| return is === 'a' && external ? 'new-window' : undefined; | ||
| }; | ||
|
|
||
| /** | ||
| * The 40px and 48px buttons pair with a 20px icon; the smaller sizes use 16px. | ||
| */ | ||
| const resolveIconSize = (size: ButtonProps['size']): IconProps['size'] => | ||
| size === undefined || size === 'large' ? 'x20' : 'x16'; | ||
|
|
||
| /** | ||
| * Indicates an actionable user action. | ||
| */ | ||
|
|
@@ -113,6 +135,14 @@ function Button({ | |
| (large && 'large') || | ||
| undefined; | ||
|
|
||
| const effectiveIcon = resolveIcon(icon, is, external); | ||
|
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 the motivation and this results in a more concise and drift proof devExp - but i'm wondering whether we shouldn't have an
Author
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 think that makes sense to allow that. |
||
| const iconSize = resolveIconSize(effectiveSize); | ||
| const hasLeadingIcon = Boolean(effectiveIcon || loading); | ||
|
|
||
| // The gap only separates the icon from a label. On an icon-only button it has | ||
| // nothing to separate and just pushes the icon off centre by half its width. | ||
| const iconGap = children ? 4 : undefined; | ||
|
|
||
| return ( | ||
| <Box | ||
| is={is} | ||
|
|
@@ -123,6 +153,7 @@ function Button({ | |
| rcx-button--medium={effectiveSize === 'medium'} | ||
| rcx-button--large={effectiveSize === 'large'} | ||
| rcx-button--square={square} | ||
| rcx-button--with-icon={hasLeadingIcon && !square} | ||
| rcx-button--tiny-square={effectiveSize === 'tiny' && square} | ||
| rcx-button--mini-square={effectiveSize === 'mini' && square} | ||
| rcx-button--small-square={effectiveSize === 'small' && square} | ||
|
|
@@ -135,10 +166,16 @@ function Button({ | |
| {...props} | ||
| > | ||
| <span className='rcx-button--content'> | ||
| {icon && !loading && ( | ||
| <Icon size='x16' name={icon} marginInlineEnd={4} /> | ||
| {effectiveIcon && !loading && ( | ||
| <Icon | ||
| size={iconSize} | ||
| name={effectiveIcon} | ||
| marginInlineEnd={iconGap} | ||
| /> | ||
| )} | ||
| {loading && ( | ||
| <Icon size={iconSize} name='loading' marginInlineEnd={iconGap} /> | ||
| )} | ||
| {loading && <Icon size='x16' name='loading' marginInlineEnd={4} />} | ||
| {children} | ||
| </span> | ||
| </Box> | ||
|
|
||
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.
No need for this
patchchangeset. We can rely on the minor changeset message here since the PR only has a few changes, that are already covered by the minor description.Please delete this file