Skip to content

Add loading support to ActionList.TrailingAction component #6239

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Jul 1, 2025
Merged

Add loading support to ActionList.TrailingAction component #6239

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
804b7d3
add loading prop
liuliu-dev Jun 25, 2025
08511be
adjust the loading button ui, restrict loading to buttons, uncomment …
liuliu-dev Jun 30, 2025
196b8c4
use base-size-12
liuliu-dev Jun 30, 2025
d78fe3d
Merge branch 'main' into liuliu/add-loading-support-to-actionlist-tra…
liuliu-dev Jun 30, 2025
783d1d5
maintain accessible contrast for labels when TrailingAction is loading
liuliu-dev Jun 30, 2025
d84e56b
improve color contrast when trailingaction is loading
liuliu-dev Jul 1, 2025
5f4fecc
muted
liuliu-dev Jul 1, 2025
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/calm-hoops-tie.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@primer/react': minor
---

Add loading support to ActionList.TrailingAction component.
8 changes: 7 additions & 1 deletion packages/react/src/ActionList/ActionList.docs.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,12 @@
"name": "href",
"type": "string",
"description": "href when the TrailingAction is rendered as a link."
},
{
"name": "loading",
"type": "boolean",
"defaultValue": "false",
"description": "Whether the TrailingAction is in a loading state. When true, the TrailingAction will render a spinner instead of an icon."
}
]
},
Expand Down Expand Up @@ -393,4 +399,4 @@
]
}
]
}
}
8 changes: 7 additions & 1 deletion packages/react/src/ActionList/TrailingAction.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,14 @@ export type ActionListTrailingActionProps = ElementProps & {
icon?: React.ElementType
label: string
className?: string
/**
* Specify whether the action is in a loading state
*/
loading?: boolean
Copy link
Member

Choose a reason for hiding this comment

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

I think we should be able to uncomment the existing WithTrailingAction story. We can adding the loading prop to one of the TrailingAction implementations in that story 😁

}

export const TrailingAction = forwardRef(
({as = 'button', icon, label, href = null, className, ...props}, forwardedRef) => {
({as = 'button', icon, label, href = null, className, loading, ...props}, forwardedRef) => {
return (
<span className={clsx(className, classes.TrailingAction)}>
{icon ? (
Expand All @@ -33,6 +37,7 @@ export const TrailingAction = forwardRef(
variant="invisible"
tooltipDirection="w"
href={href}
loading={loading}
// @ts-expect-error StyledButton wants both Anchor and Button refs
ref={forwardedRef}
className={classes.TrailingActionButton}
Expand All @@ -44,6 +49,7 @@ export const TrailingAction = forwardRef(
variant="invisible"
as={as}
Copy link
Member

Choose a reason for hiding this comment

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

Non-blocking: I'm wondering if we'd want to convert this to a button when loading state is true (e.g. as={loading ? undefined : as}). Mainly because we can't disable a link (aria-disabled is added to the loading states of IconButton). There are some accessibility considerations, such as if it's confusing to go from a button to a link once the loading state is finished. We'd also need to ensure focus remained on the element once the loading state resolves. An alternative is to only allow loading states for button trailing actions.

I'm indifferent on this though. Curious what you and @joshblack think!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer the type restriction approach - only allowing loading states for button trailing actions. The element type remains consistent, no changing from a button to a link.

href={href}
loading={loading}
Copy link
Member

Choose a reason for hiding this comment

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

I noticed when using loading without an Icon (e.g. ActionList.TrailingAction as="a" href="#" label="Some action 1" loading />) the TrailingAction takes the full width of the inner contents when loading. I'm wondering if we should add some sort of conditional or style so that the width remains consistent when loading is true.

Example:

Trailing action within ActionList, that is currently in a loading state. The action itself has a large width, due to the inner text being only visibly hidden via `visibility: hidden` rather than `display: none`

What we might want:

Trailing Action with a smaller width due to the inner contents being removed.

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've actually addressed this by aligning the spinner to the right when loading, rather than changing the button width, so the button maintains its natural width and the layout doesn't shift during the loading transition. Let me know what you think 👀

ref={forwardedRef}
className={classes.TrailingActionButton}
{...props}
Expand Down
Loading