Skip to content
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

feat: Allow Button to display an icon without a label #1654

Merged
merged 35 commits into from
Oct 23, 2024
Merged
Show file tree
Hide file tree
Changes from 33 commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
3230f5d
Changed children to label and added icons
dlnr Aug 23, 2024
14e1261
refactor: Remove unused children prop in Button component
dlnr Aug 23, 2024
d9699bb
Button label update, and allow children poc
dlnr Sep 3, 2024
51271bc
Merge branch 'develop' of https://github.com/Amsterdam/design-system …
dlnr Oct 8, 2024
6bfb91c
Updates to new changes in figma branch, revert to enum icon position
dlnr Oct 8, 2024
479bd06
Test fixes
dlnr Oct 9, 2024
6eb720a
Refactor Button component to support icon-only buttons
dlnr Oct 11, 2024
ad3dc19
Test adjustments
dlnr Oct 11, 2024
8d3e922
Revert "Test adjustments"
dlnr Oct 14, 2024
86e0b33
Revert "Refactor Button component to support icon-only buttons"
dlnr Oct 14, 2024
00c8d0b
Reverting to viewer props
dlnr Oct 14, 2024
d9319dd
Merge branch 'develop' of https://github.com/Amsterdam/design-system …
dlnr Oct 14, 2024
be63fca
Test fix
dlnr Oct 14, 2024
e42cabb
Change in documentation
dlnr Oct 14, 2024
13ef881
More docs changes
dlnr Oct 14, 2024
b313447
Button text in stories
dlnr Oct 14, 2024
724b05a
Update documentation on variants ans icons
VincentSmedinga Oct 14, 2024
0b68fb5
Suggested changes
dlnr Oct 15, 2024
e1473c9
Merge branch 'develop' of https://github.com/Amsterdam/design-system …
dlnr Oct 15, 2024
4a0f2a5
Button story fix
dlnr Oct 15, 2024
a9d7f96
Merge branch 'develop' into feature/DES-859-icon-only-button
VincentSmedinga Oct 18, 2024
a4285d6
Update storybook/src/components/Button/Button.docs.mdx
dlnr Oct 18, 2024
5ab59ca
Suggestions
dlnr Oct 18, 2024
1756232
iconProps suggestion
dlnr Oct 18, 2024
20a164b
Class change
dlnr Oct 18, 2024
8e67d2e
Use a universally recognized as per our own docs
VincentSmedinga Oct 18, 2024
e8cd4db
Fix class name
VincentSmedinga Oct 18, 2024
2a1b43f
Test icon positions with a span
dlnr Oct 21, 2024
771da4a
Icon control fix and example buttons
dlnr Oct 21, 2024
7c4a52c
Icon should only be square without a label
dlnr Oct 21, 2024
e972a60
Prevent iconposition class without icon and token naming
dlnr Oct 21, 2024
025b0fb
Refactor Button component to separate IconButton and TextButton props
dlnr Oct 21, 2024
3e2ef6c
Merge branch 'develop' into feature/DES-859-icon-only-button
VincentSmedinga Oct 21, 2024
8cf5ef6
Obsolete value for icon in stories
dlnr Oct 21, 2024
b96ebbf
Merge branch 'develop' of https://github.com/Amsterdam/design-system …
dlnr Oct 22, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions packages/css/src/components/button/button.scss
Original file line number Diff line number Diff line change
Expand Up @@ -95,3 +95,8 @@
color: var(--ams-button-tertiary-hover-color);
}
}

.ams-button--icon-position-only {
padding-block: var(--ams-button-icon-position-only-padding-block);
padding-inline: var(--ams-button-icon-position-only-padding-inline);
}
52 changes: 51 additions & 1 deletion packages/react/src/Button/Button.test.tsx
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { ShareIcon } from '@amsterdam/design-system-react-icons'
import { render, screen } from '@testing-library/react'
import '@testing-library/jest-dom'
import { createRef } from 'react'
Expand Down Expand Up @@ -104,10 +105,59 @@ describe('Button', () => {
it('is able to pass a React ref', () => {
const ref = createRef<HTMLButtonElement>()

const { container } = render(<Button ref={ref} />)
const { container } = render(<Button ref={ref}>Click me!</Button>)

const button = container.querySelector(':only-child')

expect(ref.current).toBe(button)
})

it('renders a button with an icon at the end', () => {
render(
<Button icon={ShareIcon}>
<span>Share</span>
</Button>,
)

const button = screen.getByRole('button', {
name: 'Share',
})

expect(button).toBeInTheDocument()
const icon = button.querySelector('.ams-icon:last-child')
expect(icon).toBeInTheDocument()
})

it('renders a button with an icon at the start', () => {
render(
<Button icon={ShareIcon} iconPosition="start">
<span>Share</span>
</Button>,
)

const button = screen.getByRole('button', {
name: 'Share',
})

expect(button).toBeInTheDocument()
const icon = button.querySelector('.ams-icon:first-child')
expect(icon).toBeInTheDocument()
})

it('renders a button with an icon only', () => {
render(
<Button icon={ShareIcon} iconPosition="only" variant="tertiary">
Share
</Button>,
)

const button = screen.getByRole('button', {
name: 'Share',
})

expect(button).toBeInTheDocument()
expect(button).toHaveClass('ams-button--icon-position-only')
const label = button.querySelector('.ams-visually-hidden')
expect(label).toHaveTextContent('Share')
})
})
32 changes: 28 additions & 4 deletions packages/react/src/Button/Button.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,26 +6,50 @@
import clsx from 'clsx'
import { forwardRef } from 'react'
import type { ButtonHTMLAttributes, ForwardedRef, PropsWithChildren } from 'react'
import { Icon } from '../Icon'
import type { IconProps } from '../Icon'

type IconButtonProps = {
/** An icon to add to the button. */
icon: IconProps['svg']
/** The position of the icon. The default is after the label. */
iconPosition?: 'start' | 'only'
VincentSmedinga marked this conversation as resolved.
Show resolved Hide resolved
}

type TextButtonProps = {
icon?: never
iconPosition?: never
}

export type ButtonProps = {
/** The level of prominence. Use a primary button only once per page or section. */
variant?: 'primary' | 'secondary' | 'tertiary'
} & PropsWithChildren<ButtonHTMLAttributes<HTMLButtonElement>>
} & (IconButtonProps | TextButtonProps) &
PropsWithChildren<ButtonHTMLAttributes<HTMLButtonElement>>

export const Button = forwardRef(
(
{ children, className, type, disabled, variant = 'primary', ...restProps }: ButtonProps,
{ children, className, disabled, icon, iconPosition, type, variant = 'primary', ...restProps }: ButtonProps,
ref: ForwardedRef<HTMLButtonElement>,
) => {
return (
<button
{...restProps}
ref={ref}
disabled={disabled}
className={clsx('ams-button', `ams-button--${variant}`, className)}
className={clsx(
'ams-button',
`ams-button--${variant}`,
icon && iconPosition === 'only' && `ams-button--icon-position-only`,
className,
)}
type={type || 'button'}
>
{children}
{icon && (iconPosition === 'start' || iconPosition === 'only') && (
<Icon svg={icon} size="level-5" square={iconPosition === 'only'} />
dlnr marked this conversation as resolved.
Show resolved Hide resolved
)}
{icon && iconPosition === 'only' ? <span className="ams-visually-hidden">{children}</span> : children}
{icon && !iconPosition && <Icon svg={icon} size="level-5" />}
</button>
)
},
Expand Down
4 changes: 4 additions & 0 deletions proprietary/tokens/src/components/ams/button.tokens.json
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,10 @@
"background-color": { "value": "transparent" },
"color": { "value": "{ams.color.neutral-grey2}" }
}
},
"icon-position-only": {
"padding-block": { "value": "{ams.space.sm}" },
"padding-inline": { "value": "{ams.space.sm}" }
}
}
}
Expand Down
29 changes: 23 additions & 6 deletions storybook/src/components/Button/Button.docs.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -16,28 +16,45 @@ import README from "../../../../packages/css/src/components/button/README.md?raw

### Primary

The most important call-to-action.
One primary Button may be used per screen.
A primary button draws attention to the most important call to action.
Only one primary Button should be used per screen.

<Canvas of={ButtonStories.Primary} />

### Secondary

Use the secondary Button for less prominent calls to action.
It is possible to use more than 1 secondary Button.
Use a secondary button for other actions.

<Canvas of={ButtonStories.Secondary} />

### Tertiary

Use tertiary Buttons for unimportant calls to action – as many as necessary.
Tertiary buttons can be used to distinguish their importance from secondary buttons.
They are also a good choice for buttons with an icon only.

<Canvas of={ButtonStories.Tertiary} />

### With icon
### With an icon

Add an icon if it makes it easier or faster to understand its purpose.
The icon will appear after the label.

<Canvas of={ButtonStories.WithIcon} />

### With an icon at the start

The icon can also appear before the label.

<Canvas of={ButtonStories.WithIconAtStart} />

### With an icon only

A button can even consist of an icon only.
Do this only for icons that are universally recognized.
You must still provide a label – it will be used to make the button accessible.

<Canvas of={ButtonStories.WithIconOnly} />

### Text wrapping

Keep in mind that the label may occasionally wrap over multiple lines:
Expand Down
53 changes: 40 additions & 13 deletions storybook/src/components/Button/Button.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,27 +3,39 @@
* Copyright Gemeente Amsterdam
*/

import { Icon } from '@amsterdam/design-system-react'
import { Button } from '@amsterdam/design-system-react/src'
import { ShareIcon } from '@amsterdam/design-system-react-icons'
import * as Icons from '@amsterdam/design-system-react-icons'
import { Meta, StoryObj } from '@storybook/react'

const meta = {
title: 'Components/Buttons/Button',
component: Button,
args: {
children: 'Button label',
variant: 'primary',
children: 'Versturen',
disabled: false,
variant: 'primary',
/* This is the only was storybook will honor the conditional in the iconPosition argType (line 29) */
// @ts-ignore
icon: null,
dlnr marked this conversation as resolved.
Show resolved Hide resolved
},
argTypes: {
VincentSmedinga marked this conversation as resolved.
Show resolved Hide resolved
children: {
description: 'The text for the label and/or an icon.',
table: { disable: false },
},
disabled: {
description: 'Prevents interaction. Avoid if possible.',
},
icon: {
control: {
type: 'select',
},
options: Object.keys(Icons),
mapping: Icons,
},
iconPosition: {
control: {
type: 'inline-radio',
labels: { undefined: 'end', start: 'start', only: 'only' },
},
options: [undefined, 'start', 'only'],
},
},
} satisfies Meta<typeof Button>

Expand All @@ -35,24 +47,39 @@ export const Primary: Story = {}

export const Secondary: Story = {
args: {
children: 'Annuleren',
variant: 'secondary',
},
}

export const Tertiary: Story = {
args: {
children: 'Terug',
variant: 'tertiary',
},
}

export const WithIcon: Story = {
args: {
children: ['Button label', <Icon key="icon" svg={ShareIcon} size="level-5" />],
children: 'Sluiten',
icon: Icons.CloseIcon,
},
argTypes: {
children: {
table: { disable: true },
},
}

export const WithIconAtStart: Story = {
args: {
children: 'Sluiten',
icon: Icons.CloseIcon,
iconPosition: 'start',
},
}

export const WithIconOnly: Story = {
args: {
children: 'Sluiten',
icon: Icons.CloseIcon,
iconPosition: 'only',
variant: 'tertiary',
},
}

Expand Down