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

Un-revert "Add loading prop for Button and IconButton (#3582)" #4485

Merged
merged 47 commits into from
Jul 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
47 commits
Select commit Hold shift + click to select a range
60b3253
Revert "Revert "Add `loading` prop for `Button` and `IconButton` (#35…
siddharthkp Apr 10, 2024
0c29ea7
only overrides btn label when loading
mperrotti Apr 10, 2024
27a7c58
updates snapshots after merging from main
mperrotti Apr 10, 2024
14c0278
Merge branch 'main' into revert-4464-revert-3582
mperrotti Apr 11, 2024
ac0801b
Merge branch 'main' into revert-4464-revert-3582
mperrotti Apr 12, 2024
708f452
Merge branch 'main' of github.com:primer/react into revert-4464-rever…
mperrotti Apr 19, 2024
e765c05
update tooltip tests to accomodate loading message aria-describedby
mperrotti Apr 19, 2024
22ea550
Merge branch 'revert-4464-revert-3582' of github.com:primer/react int…
mperrotti Apr 19, 2024
b881fd7
import missing modules in IconButton stories
mperrotti Apr 22, 2024
ccde61a
Merge branch 'main' of github.com:primer/react into revert-4464-rever…
mperrotti Apr 22, 2024
3399501
makes aria-labelledby prop remain set when when button is in a loadin…
mperrotti Apr 26, 2024
4fce38a
Merge branch 'main' of github.com:primer/react into revert-4464-rever…
mperrotti Apr 26, 2024
c454b3e
Update packages/react/src/Button/Button.examples.stories.tsx
mperrotti Jun 11, 2024
29e5955
Update packages/react/src/Button/Button.examples.stories.tsx
mperrotti Jun 11, 2024
2e1df44
Merge branch 'main' of github.com:primer/react into revert-4464-rever…
mperrotti Jun 17, 2024
86b14e7
Merge branch 'revert-4464-revert-3582' of github.com:primer/react int…
mperrotti Jun 17, 2024
852c9e2
Merge branch 'main' of github.com:primer/react into revert-4464-rever…
mperrotti Jun 17, 2024
f7616c9
updates textinput snapshots
mperrotti Jun 17, 2024
f9cdfb5
addresses remaining PR feedback
mperrotti Jun 17, 2024
1ee5854
appeases the linter, updates snaps
mperrotti Jun 17, 2024
2749e52
leaves loading prop undefined so we do not always render the wrapper
mperrotti Jun 17, 2024
44845b3
test(vrt): update snapshots
mperrotti Jun 17, 2024
b3f90ed
Merge branch 'main' into revert-4464-revert-3582
mperrotti Jun 19, 2024
59d90c6
Merge branch 'main' into revert-4464-revert-3582
mperrotti Jun 19, 2024
f61d1d6
Merge branch 'main' into revert-4464-revert-3582
mperrotti Jun 19, 2024
95843eb
Merge branch 'main' of github.com:primer/react into revert-4464-rever…
mperrotti Jun 20, 2024
73cf068
Merge branch 'main' into revert-4464-revert-3582
siddharthkp Jun 26, 2024
3d2f917
Merge branch 'main' into revert-4464-revert-3582
siddharthkp Jun 28, 2024
78e99a3
trying again without changing colors on faux-disabled buttons
mperrotti Jul 3, 2024
9007105
Merge branch 'revert-4464-revert-3582' of github.com:primer/react int…
mperrotti Jul 3, 2024
e9e391a
Merge branch 'main' of github.com:primer/react into revert-4464-rever…
mperrotti Jul 3, 2024
d401782
replaces Status with AriaStatus
mperrotti Jul 3, 2024
8cb4830
replaces one more Status with AriaStatus
mperrotti Jul 3, 2024
50cfa75
rms added aria-disabled styles
mperrotti Jul 3, 2024
be94467
test(vrt): update snapshots
mperrotti Jul 3, 2024
76724c7
Merge branch 'main' into revert-4464-revert-3582
mperrotti Jul 8, 2024
a7741ba
Merge branch 'main' into revert-4464-revert-3582
mperrotti Jul 10, 2024
abbd4d8
Merge branch 'main' of github.com:primer/react into revert-4464-rever…
mperrotti Jul 12, 2024
f379211
Update icon button tests to make it work with the loading state
broccolinisoup Jul 16, 2024
c1b40a2
Merge branch 'main' into revert-4464-revert-3582
siddharthkp Jul 19, 2024
093a8bd
add story with leading visual and count
siddharthkp Jul 22, 2024
50eb950
Merge branch 'main' of github.com:primer/react into revert-4464-rever…
mperrotti Jul 23, 2024
a400ed9
misc bugfixes:
mperrotti Jul 23, 2024
abd6870
Merge branch 'revert-4464-revert-3582' of github.com:primer/react int…
mperrotti Jul 23, 2024
85d8358
test(vrt): update snapshots
mperrotti Jul 23, 2024
691bb7a
Merge branch 'main' into revert-4464-revert-3582
mperrotti Jul 24, 2024
85c68a6
Merge branch 'main' into revert-4464-revert-3582
mperrotti Jul 25, 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 .changeset/lazy-jobs-pump.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@primer/react": minor
---

Add `loading` state to `Button` and `IconButton`
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
178 changes: 178 additions & 0 deletions e2e/components/Button.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -479,6 +479,184 @@ test.describe('Button', () => {
}
})

test.describe('Loading', () => {
for (const theme of themes) {
test.describe(theme, () => {
test('default @vrt', async ({page}) => {
await visit(page, {
id: 'components-button-features--loading',
globals: {
colorScheme: theme,
},
})

// Default state
expect(await page.screenshot({animations: 'disabled'})).toMatchSnapshot(`Button.Loading.${theme}.png`)
})

test('axe @aat', async ({page}) => {
await visit(page, {
id: 'components-button-features--loading',
globals: {
colorScheme: theme,
},
})
await expect(page).toHaveNoViolations({
rules: {
'color-contrast': {
enabled: theme !== 'dark_dimmed',
},
},
})
})
})
}
})

test.describe('Loading Custom Announcement', () => {
for (const theme of themes) {
test.describe(theme, () => {
test('default @vrt', async ({page}) => {
await visit(page, {
id: 'components-button-features--loading-custom-announcement',
globals: {
colorScheme: theme,
},
})

// Default state
expect(await page.screenshot({animations: 'disabled'})).toMatchSnapshot(
`Button.Loading Custom Announcement.${theme}.png`,
)
})

test('axe @aat', async ({page}) => {
await visit(page, {
id: 'components-button-features--loading-custom-announcement',
globals: {
colorScheme: theme,
},
})
await expect(page).toHaveNoViolations({
rules: {
'color-contrast': {
enabled: theme !== 'dark_dimmed',
},
},
})
})
})
}
})

test.describe('Loading With Leading Visual', () => {
for (const theme of themes) {
test.describe(theme, () => {
test('default @vrt', async ({page}) => {
await visit(page, {
id: 'components-button-features--loading-with-leading-visual',
globals: {
colorScheme: theme,
},
})

// Default state
expect(await page.screenshot({animations: 'disabled'})).toMatchSnapshot(
`Button.Loading With Leading Visual.${theme}.png`,
)
})

test('axe @aat', async ({page}) => {
await visit(page, {
id: 'components-button-features--loading-with-leading-visual',
globals: {
colorScheme: theme,
},
})
await expect(page).toHaveNoViolations({
rules: {
'color-contrast': {
enabled: theme !== 'dark_dimmed',
},
},
})
})
})
}
})

test.describe('Loading With Trailing Visual', () => {
for (const theme of themes) {
test.describe(theme, () => {
test('default @vrt', async ({page}) => {
await visit(page, {
id: 'components-button-features--loading-with-trailing-visual',
globals: {
colorScheme: theme,
},
})

// Default state
expect(await page.screenshot({animations: 'disabled'})).toMatchSnapshot(
`Button.Loading With Trailing Visual.${theme}.png`,
)
})

test('axe @aat', async ({page}) => {
await visit(page, {
id: 'components-button-features--loading-with-trailing-visual',
globals: {
colorScheme: theme,
},
})
await expect(page).toHaveNoViolations({
rules: {
'color-contrast': {
enabled: theme !== 'dark_dimmed',
},
},
})
})
})
}
})

test.describe('Loading With Trailing Action', () => {
for (const theme of themes) {
test.describe(theme, () => {
test('default @vrt', async ({page}) => {
await visit(page, {
id: 'components-button-features--loading-with-trailing-action',
globals: {
colorScheme: theme,
},
})

// Default state
expect(await page.screenshot({animations: 'disabled'})).toMatchSnapshot(
`Button.Loading With Trailing Action.${theme}.png`,
)
})

test('axe @aat', async ({page}) => {
await visit(page, {
id: 'components-button-features--loading-with-trailing-action',
globals: {
colorScheme: theme,
},
})
await expect(page).toHaveNoViolations({
rules: {
'color-contrast': {
enabled: theme !== 'dark_dimmed',
},
},
})
})
})
}
})

test.describe('Dev: Invisible Variants', () => {
for (const theme of themes) {
test.describe(theme, () => {
Expand Down
1 change: 1 addition & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

16 changes: 11 additions & 5 deletions packages/react/src/Button/Button.docs.json
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,6 @@
"type": "number | string",
"description": "For counter buttons, the number to display."
},
{
"name": "disabled",
"type": "boolean",
"description": "Items that are disabled can not be clicked, selected, or navigated through."
},
{
"name": "inactive",
"type": "boolean",
Expand All @@ -53,6 +48,17 @@
"type": "React.ElementType",
"description": "A visual to display before the button text."
},
{
"name": "loading",
"type": "boolean",
"description": "When true, the button is in a loading state."
},
{
"name": "loadingAnnouncement",
"type": "string",
"description": "The content to announce to screen readers when loading. This requires `loading` prop to be true"
},

{
"name": "ref",
"type": "React.RefObject<HTMLButtonElement>"
Expand Down
79 changes: 79 additions & 0 deletions packages/react/src/Button/Button.examples.stories.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
import React from 'react'
import type {Meta} from '@storybook/react'
import {Button} from '.'
import {DownloadIcon} from '@primer/octicons-react'
import {Banner} from '../drafts'
import {AriaStatus} from '../live-region'

const meta: Meta<typeof Button> = {
title: 'Components/Button/Examples',
} as Meta<typeof Button>

export default meta

export const LoadingStatusAnnouncementSuccessful = () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In these examples, when the loading state is activated, the focus gets lost and removed from the button. I believe this is a 2.4.3 issue, and may result in some screen reader users ending up at the top of the page. Could we avoid focus loss?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. I'll take a look.

const [loading, setLoading] = React.useState(false)
const [success, setSuccess] = React.useState(false)

const resolveAction = async () => {
setLoading(true)
await new Promise(resolve => setTimeout(resolve, 1500))
setLoading(false)

return await true
}

const onClick = (resolveType: 'error' | 'success') => async () => {
const actionResult = await resolveAction()

if (resolveType === 'error') {
setSuccess(!actionResult)
return
}

setSuccess(actionResult)
}

return (
<>
<AriaStatus>{!loading && success ? 'Export completed' : null}</AriaStatus>
<Button loading={loading} leadingVisual={DownloadIcon} onClick={onClick('success')}>
Export (success)
</Button>
</>
)
}

export const LoadingStatusAnnouncementError = () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this example, the failure state is communicated to assistive technologies via announcement, but there's no visual indication that anything failed.

Do we have guidance around communicating errors visually? If so, should this example include that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great point! I'll take a look through our docs and update this.

const [loading, setLoading] = React.useState(false)
const [error, setError] = React.useState(false)

const resolveAction = async () => {
setLoading(true)
await new Promise(resolve => setTimeout(resolve, 1500))
setLoading(false)

return await true
}

const onClick = (resolveType: 'error' | 'success') => async () => {
const actionResult = await resolveAction()

if (resolveType === 'error') {
setError(actionResult)
return
}

setError(!actionResult)
}

return (
<>
{!loading && error ? <Banner title="Export failed" variant="critical" /> : null}

<Button loading={loading} leadingVisual={DownloadIcon} onClick={onClick('error')}>
Export (error)
</Button>
</>
)
}
42 changes: 41 additions & 1 deletion packages/react/src/Button/Button.features.stories.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import {EyeIcon, TriangleDownIcon, HeartIcon, CommentIcon} from '@primer/octicons-react'
import {EyeIcon, TriangleDownIcon, HeartIcon, DownloadIcon, CommentIcon} from '@primer/octicons-react'
import React, {useState} from 'react'
import {Button} from '.'
import {Stack} from '../Stack/Stack'
Expand Down Expand Up @@ -141,6 +141,46 @@ export const Medium = () => <Button size="medium">Default</Button>

export const Large = () => <Button size="large">Default</Button>

export const Loading = () => <Button loading>Default</Button>

export const LoadingCustomAnnouncement = () => (
<Button loading loadingAnnouncement="This is a custom loading announcement">
Default
</Button>
)

export const LoadingWithLeadingVisual = () => (
<Button loading leadingVisual={DownloadIcon}>
Export
</Button>
)

export const LoadingWithTrailingVisual = () => (
<Button loading trailingVisual={DownloadIcon}>
Export
</Button>
)

export const LoadingWithTrailingAction = () => (
<Button loading trailingAction={TriangleDownIcon}>
Export dropdown
</Button>
)

export const LoadingTrigger = () => {
const [isLoading, setIsLoading] = useState(false)

const handleClick = () => {
setIsLoading(true)
}

return (
<Button loading={isLoading} onClick={handleClick} leadingVisual={DownloadIcon}>
Export
</Button>
)
}

export const LabelWrap = () => {
return (
<Stack style={{width: '200px'}}>
Expand Down
11 changes: 11 additions & 0 deletions packages/react/src/Button/Button.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -48,11 +48,21 @@ Playground.argTypes = {
type: 'boolean',
},
},
loading: {
control: {
type: 'boolean',
},
},
labelWrap: {
control: {
type: 'boolean',
},
},
count: {
control: {
type: 'number',
},
},
leadingVisual: OcticonArgType([EyeClosedIcon, EyeIcon, SearchIcon, XIcon, HeartIcon]),
trailingVisual: OcticonArgType([EyeClosedIcon, EyeIcon, SearchIcon, XIcon, HeartIcon]),
trailingAction: OcticonArgType([TriangleDownIcon]),
Expand All @@ -64,6 +74,7 @@ Playground.args = {
inactive: false,
variant: 'default',
alignContent: 'center',
loading: false,
trailingVisual: null,
leadingVisual: null,
trailingAction: null,
Expand Down
Loading
Loading