-
Notifications
You must be signed in to change notification settings - Fork 535
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
Adds loading state to ActionList items #4051
Conversation
🦋 Changeset detectedLatest commit: 2c2f814 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
This will be a draft until #3913 is merged |
size-limit report 📦
|
b6b3884
to
6906bec
Compare
Updating snapshots since we updated the stories. |
…into mp/loading-actionlist-item
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.
I have two suggestions, but otherwise looks good!
export const WithLoadingItems: StoryFn = () => ( | ||
<PageLayout> | ||
<PageLayout.Pane position="start"> | ||
<NavList> | ||
<NavList.Item href="#" loading> |
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.
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.
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.
Thanks - I missed this one.
@@ -456,7 +428,8 @@ export const Item = React.forwardRef<HTMLLIElement, ActionListItemProps>( | |||
{slots.blockDescription} | |||
</Box> | |||
</ItemWrapper> | |||
{!inactive && !menuContext && Boolean(slots.trailingAction) && slots.trailingAction} | |||
{!inactive && !loading && !menuContext && Boolean(slots.trailingAction) && slots.trailingAction} | |||
{loading === true && <VisuallyHidden>Loading</VisuallyHidden>} |
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.
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.
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.
Ahh ok I understand.
Relates to https://github.com/github/primer/issues/2681
Kapture.2023-12-11.at.13.38.49.mp4
Changelog
New
ActionList.item
acceptsloading
propChanged
Removed
Rollout strategy
Testing & Reviewing
Merge checklist