Skip to content

Adds loading state to ActionList items - #4051

Merged
mperrotti merged 33 commits into
mainfrom
mp/loading-actionlist-item
Jul 5, 2024
Merged

Adds loading state to ActionList items#4051
mperrotti merged 33 commits into
mainfrom
mp/loading-actionlist-item

Conversation

@mperrotti

@mperrottimperrotti commented Dec 11, 2023

Copy link
Copy Markdown
Contributor

Relates to https://github.com/github/primer/issues/2681

Kapture.2023-12-11.at.13.38.49.mp4

Changelog

New

  • ActionList.item accepts loading prop

Changed

Removed

Rollout strategy

  • Patch release
  • Minor release
  • Major release; if selected, include a written rollout or migration plan
  • None; if selected, include a brief description as to why

Testing & Reviewing

Merge checklist

@changeset-bot

changeset-botBot commented Dec 11, 2023

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 2c2f814

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
NameType
@primer/reactMinor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@mperrotti

Copy link
Copy Markdown
ContributorAuthor

This will be a draft until #3913 is merged

@github-actions

github-actionsBot commented Dec 11, 2023

Copy link
Copy Markdown
Contributor

size-limit report 📦

PathSize
packages/react/dist/browser.esm.js91.25 KB (+0.16% 🔺)
packages/react/dist/browser.umd.js91.51 KB (+0.19% 🔺)

@mperrotti
mperrottiforce-pushed the mp/loading-actionlist-item branch from b6b3884 to 6906becCompareDecember 12, 2023 14:44
@mperrotti
mperrotti marked this pull request as ready for review December 12, 2023 14:44
@mperrotti
mperrotti requested review from a team and broccolinisoupDecember 12, 2023 14:44
@github-actions
github-actionsBottemporarily deployed to storybook-preview-4051 December 12, 2023 14:48 Inactive
@mperrottimperrotti added the update snapshots 🤖 Command that updates VRT snapshots on the pull request label Dec 12, 2023
@github-actionsgithub-actionsBot removed the update snapshots 🤖 Command that updates VRT snapshots on the pull request label Dec 12, 2023
@github-actions
github-actionsBottemporarily deployed to storybook-preview-4051 December 12, 2023 15:10 Inactive
@github-actions
github-actionsBottemporarily deployed to storybook-preview-4051 December 18, 2023 20:45 Inactive
@mperrottimperrotti added the update snapshots 🤖 Command that updates VRT snapshots on the pull request label Dec 18, 2023
@github-actions
github-actionsBottemporarily deployed to storybook-preview-4051 July 3, 2024 14:23 Inactive
@github-actions
github-actionsBottemporarily deployed to storybook-preview-4051 July 3, 2024 15:42 Inactive
@github-actions
github-actionsBottemporarily deployed to storybook-preview-4051 July 3, 2024 19:51 Inactive
@github-actions
github-actionsBottemporarily deployed to storybook-preview-4051 July 3, 2024 20:01 Inactive
@mperrotti

Copy link
Copy Markdown
ContributorAuthor

Updating snapshots since we updated the stories.

@mperrottimperrotti added the update snapshots 🤖 Command that updates VRT snapshots on the pull request label Jul 3, 2024
@github-actionsgithub-actionsBot removed the update snapshots 🤖 Command that updates VRT snapshots on the pull request label Jul 3, 2024
@primer
primerBottemporarily deployed to github-pages July 3, 2024 20:26 Inactive
@github-actions
github-actionsBottemporarily deployed to storybook-preview-4051 July 3, 2024 20:27 Inactive
@github-actions
github-actionsBottemporarily deployed to storybook-preview-4051 July 3, 2024 20:59 Inactive

@TylerJDevTylerJDev left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I have two suggestions, but otherwise looks good!

Comment on lines +228 to +232
export const WithLoadingItems: StoryFn = () => (
<PageLayout>
<PageLayout.Pane position="start">
<NavList>
<NavList.Item href="#" loading>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Now that we don't have loading for links, should this prop & story be removed from NavList? I noticed that this story wasn't working, and I think it's because all NavList.Item(s) utilize ActionList.LinkItem by default.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Thanks - I missed this one.

</ItemWrapper>
{!inactive && !menuContext && Boolean(slots.trailingAction) && slots.trailingAction}
{!inactive && !loading && !menuContext && Boolean(slots.trailingAction) && slots.trailingAction}
{loading === true && <VisuallyHidden>Loading</VisuallyHidden>}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we might need to put this in the Box above so that it is referenced to via aria-labelledby, or we could include it in the aria-labelledby list and generate a unique ID for the VisuallyHidden component that we could add to the list.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Ahh ok I understand.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@mperrotti@emilybrick@broccolinisoup@TylerJDev