Skip to content

Commit

Permalink
Adds loading state to ActionList items (#4051)
Browse files Browse the repository at this point in the history
* adds loading state to inactive items

* adds changeset

* test(vrt): update snapshots

* test(vrt): update snapshots

* moves Spinner changes to a different branch

* test(vrt): update snapshots

* addresses a11y feedback about tooltip label and description

* update outdated Tooltip import

* attempt to fix circular dependency

* another attempt at resolving circular dependency

* fixes merge mistakes

* addresses PR feedback

* rm loading link item from feature story

* test(vrt): update snapshots

* rm 'default state test' assertion for ActionMenus w/ loading and inactive items

* addresses last bit of PR feedback

---------

Co-authored-by: mperrotti <mperrotti@users.noreply.github.com>
  • Loading branch information
mperrotti and mperrotti committed Jul 5, 2024
1 parent 199e384 commit 7e644b7
Show file tree
Hide file tree
Showing 79 changed files with 461 additions and 150 deletions.
7 changes: 7 additions & 0 deletions .changeset/lucky-squids-happen.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
'@primer/react': minor
---

Adds a loading state to ActionList items. Also allows the Spinner component to accept screenreader text.

<!-- Changed components: ActionList, ActionMenu, NavList, Spinners -->
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.
30 changes: 30 additions & 0 deletions e2e/components/ActionList.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -515,6 +515,36 @@ test.describe('ActionList', () => {
}
})

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

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

test('axe @aat', async ({page}) => {
await visit(page, {
id: 'components-actionlist-features--loading-item',
globals: {
colorScheme: theme,
},
})
await expect(page).toHaveNoViolations()
})
})
}
})

test.describe('Group With Filled Title', () => {
for (const theme of themes) {
test.describe(theme, () => {
Expand Down
30 changes: 30 additions & 0 deletions e2e/components/ActionMenu.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,36 @@ test.describe('ActionMenu', () => {
}
})

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

// Open menu
await page.locator('button', {hasText: 'Open menu'}).waitFor()
await page.getByRole('button', {name: 'Open menu'}).click()
expect(await page.screenshot({animations: 'disabled'})).toMatchSnapshot()
})

test('axe @aat', async ({page}) => {
await visit(page, {
id: 'components-actionmenu-features--loading-items',
globals: {
colorScheme: theme,
},
})
await expect(page).toHaveNoViolations()
})
})
}
})

test.describe('Multi Select', () => {
for (const theme of themes) {
test.describe(theme, () => {
Expand Down
5 changes: 5 additions & 0 deletions packages/react/src/ActionList/ActionList.docs.json
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,11 @@
"defaultValue": "",
"description": "Text describing why the item is inactive. This may be used when an item's usual functionality is unavailable due to a system error such as a database outage. \nIf there is a leading visual, the alert icon will replace the leading visual. \n If there is a trailing visual, it will replace the trailing visual.\n If there is no visual passed, it will be shown in the trailing visual slot to preserve left alignment of item content. \nText will appear in a tooltip triggered by the alert icon in ActionList items, but text will appear below the description or title on ActionMenu items."
},
{
"name": "loading",
"type": "boolean",
"description": "Whether the item is loading."
},
{
"name": "role",
"type": "AriaRole",
Expand Down
39 changes: 39 additions & 0 deletions packages/react/src/ActionList/ActionList.examples.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -456,6 +456,7 @@ export function AllCombinations(): JSX.Element {
<StarIcon />
</ActionList.TrailingVisual>
</ActionList.Item>
{/* Inactive items */}
<ActionList.Item inactiveText="Unavailable due to an outage">
L + B + T<ActionList.Description variant="block">Block description</ActionList.Description>
</ActionList.Item>
Expand Down Expand Up @@ -493,6 +494,44 @@ export function AllCombinations(): JSX.Element {
<StarIcon />
</ActionList.TrailingVisual>
</ActionList.Item>
{/* Loading items */}
<ActionList.Item loading>
L + B + T<ActionList.Description variant="block">Block description</ActionList.Description>
</ActionList.Item>
<ActionList.Item loading>
L + B + T<ActionList.Description variant="inline">Inline description</ActionList.Description>
</ActionList.Item>
<ActionList.Item loading>
<ActionList.LeadingVisual>
<StarIcon />
</ActionList.LeadingVisual>
L + I + T<ActionList.Description variant="inline">inline description</ActionList.Description>
<ActionList.TrailingVisual>
<StarIcon />
</ActionList.TrailingVisual>
</ActionList.Item>
<ActionList.Item loading>
<ActionList.LeadingVisual>
<StarIcon />
</ActionList.LeadingVisual>
L + B + T<ActionList.Description variant="block">Block description</ActionList.Description>
<ActionList.TrailingVisual>
<StarIcon />
</ActionList.TrailingVisual>
</ActionList.Item>
<ActionList.Item loading>
L + B + T<ActionList.Description variant="block">Block description</ActionList.Description>
<ActionList.TrailingVisual>
<StarIcon />
</ActionList.TrailingVisual>
</ActionList.Item>
<ActionList.Item loading>
I + B + T<ActionList.Description variant="inline">inline description</ActionList.Description>
<ActionList.Description variant="block">Block description</ActionList.Description>
<ActionList.TrailingVisual>
<StarIcon />
</ActionList.TrailingVisual>
</ActionList.Item>
<ActionList.Item variant="danger">
<ActionList.LeadingVisual>
<StarIcon />
Expand Down
13 changes: 13 additions & 0 deletions packages/react/src/ActionList/ActionList.features.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -442,6 +442,19 @@ export const InactiveItem = () => {
)
}

export const LoadingItem = () => {
return (
<ActionList aria-label="Project">
{projects.map((project, index) => (
<ActionList.Item key={index} loading={index === 1}>
{project.name}
<ActionList.Description variant="block">{project.scope}</ActionList.Description>
</ActionList.Item>
))}
</ActionList>
)
}

export const Links = () => (
<>
<ActionList.Heading as="h1" sx={{fontSize: 1}}>
Expand Down
12 changes: 12 additions & 0 deletions packages/react/src/ActionList/ActionList.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,11 @@ ItemPlayground.argTypes = {
},
options: icons,
},
loading: {
control: {
type: 'boolean',
},
},
trailingVisual: {
control: {
type: 'select',
Expand All @@ -164,6 +169,7 @@ ItemPlayground.args = {
variant: 'default',
id: 'item-1',
leadingVisual: null,
loading: false,
trailingVisual: null,
selectionVariant: 'single',
}
Expand Down Expand Up @@ -216,6 +222,7 @@ LinkItemPlayground.args = {
id: 'item-1',
inactiveText: '',
leadingVisual: null,
loading: false,
trailingVisual: null,
}
LinkItemPlayground.argTypes = {
Expand All @@ -241,6 +248,11 @@ LinkItemPlayground.argTypes = {
},
options: icons,
},
loading: {
control: {
type: 'boolean',
},
},
trailingVisual: {
control: {
type: 'select',
Expand Down
44 changes: 40 additions & 4 deletions packages/react/src/ActionList/ActionList.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,13 @@ const projects = [
{name: 'Primer React', scope: 'github/primer'},
{name: 'Disabled Project', scope: 'github/primer', disabled: true},
{name: 'Inactive Project', scope: 'github/primer', inactiveText: 'Unavailable due to an outage'},
{name: 'Loading Project', scope: 'github/primer', loading: true},
{
name: 'Inactive and Loading Project',
scope: 'github/primer',
loading: true,
inactiveText: 'Unavailable due to an outage, but loading still passed',
},
]
function SingleSelectListStory(): JSX.Element {
const [selectedIndex, setSelectedIndex] = React.useState(0)
Expand All @@ -49,6 +56,7 @@ function SingleSelectListStory(): JSX.Element {
onSelect={() => setSelectedIndex(index)}
disabled={project.disabled}
inactiveText={project.inactiveText}
loading={project.loading}
>
{project.name}
</ActionList.Item>
Expand Down Expand Up @@ -145,6 +153,24 @@ describe('ActionList', () => {
expect(options[3]).toHaveAttribute('aria-selected', 'false')
})

it('should skip onSelect on loading items', async () => {
const component = HTMLRender(<SingleSelectListStory />)
const options = await waitFor(() => component.getAllByRole('option'))

expect(options[0]).toHaveAttribute('aria-selected', 'true')
expect(options[4]).toHaveAttribute('aria-selected', 'false')

fireEvent.click(options[4])

expect(options[0]).toHaveAttribute('aria-selected', 'true')
expect(options[4]).toHaveAttribute('aria-selected', 'false')

fireEvent.keyPress(options[3], {key: 'Enter', charCode: 13})

expect(options[0]).toHaveAttribute('aria-selected', 'true')
expect(options[4]).toHaveAttribute('aria-selected', 'false')
})

it('should throw when selected is provided without a selectionVariant on parent', async () => {
// we expect console.error to be called, so we suppress that in the test
const mockError = jest.spyOn(console, 'error').mockImplementation(() => jest.fn())
Expand Down Expand Up @@ -178,10 +204,20 @@ describe('ActionList', () => {

it('should focus the button around the leading visual when tabbing to an inactive item', async () => {
const component = HTMLRender(<SingleSelectListStory />)
const inactiveOptionButton = await waitFor(() =>
component.getByRole('button', {description: projects[3].inactiveText}),
)
const inactiveIndex = projects.findIndex(project => 'inactiveText' in project)
const inactiveOptionButton = await waitFor(() => component.getByRole('button', {name: projects[3].inactiveText}))
const inactiveIndex = projects.findIndex(project => project.inactiveText === projects[3].inactiveText)

for (let i = 0; i < inactiveIndex; i++) {
await userEvent.tab()
}

expect(inactiveOptionButton).toHaveFocus()
})

it('should behave as inactive if both inactiveText and loading props are passed', async () => {
const component = HTMLRender(<SingleSelectListStory />)
const inactiveOptionButton = await waitFor(() => component.getByRole('button', {name: projects[5].inactiveText}))
const inactiveIndex = projects.findIndex(project => project.inactiveText === projects[5].inactiveText)

for (let i = 0; i < inactiveIndex; i++) {
await userEvent.tab()
Expand Down
Loading

0 comments on commit 7e644b7

Please sign in to comment.