-
Notifications
You must be signed in to change notification settings - Fork 677
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.
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
Changes from all commits
804b7d3
08511be
196b8c4
d78fe3d
783d1d5
d84e56b
5f4fecc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| '@primer/react': minor | ||
| --- | ||
|
|
||
| Add loading support to ActionList.TrailingAction component. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,10 +9,16 @@ type ElementProps = | |
| | { | ||
| as?: 'button' | ||
| href?: never | ||
| /** | ||
| * Specify whether the action is in a loading state. | ||
| * Only available for button elements. | ||
| */ | ||
| loading?: boolean | ||
| } | ||
| | { | ||
| as: 'a' | ||
| href: string | ||
| loading?: never | ||
| } | ||
|
|
||
| export type ActionListTrailingActionProps = ElementProps & { | ||
|
|
@@ -22,7 +28,7 @@ export type ActionListTrailingActionProps = ElementProps & { | |
| } | ||
|
|
||
| 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 ? ( | ||
|
|
@@ -33,6 +39,8 @@ export const TrailingAction = forwardRef( | |
| variant="invisible" | ||
| tooltipDirection="w" | ||
| href={href} | ||
| loading={loading} | ||
| data-loading={Boolean(loading)} | ||
| // @ts-expect-error StyledButton wants both Anchor and Button refs | ||
| ref={forwardedRef} | ||
| className={classes.TrailingActionButton} | ||
|
|
@@ -44,6 +52,8 @@ export const TrailingAction = forwardRef( | |
| variant="invisible" | ||
| as={as} | ||
| href={href} | ||
| loading={loading} | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I noticed when using Example:
What we might want:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 👀 |
||
| data-loading={Boolean(loading)} | ||
| ref={forwardedRef} | ||
| className={classes.TrailingActionButton} | ||
| {...props} | ||
|
|
||


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.
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-disabledis added to the loading states ofIconButton). 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!
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 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.