Add loading support to ActionList.TrailingAction component #6239
Conversation
🦋 Changeset detectedLatest commit: 804b7d3 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 |
github-actions
bot
added
the
integration-tests: recommended
|
👋 Hi, this pull request contains changes to the source code that github/github depends on. If you are GitHub staff, we recommend testing these changes with github/github using the integration workflow. Thanks! |
There was a problem hiding this comment.
Pull Request Overview
This PR adds a loading state to the ActionList.TrailingAction component so that consumers can show a spinner in place of the icon when an action is in progress.
- Introduces an optional
loadingprop in the component’s TypeScript interface - Passes the
loadingprop down to theStyledButtoninstances - Updates the JSON docs and changelog entry
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/react/src/ActionList/TrailingAction.tsx | Added loading?: boolean to props and forwarded it to the button |
| packages/react/src/ActionList/ActionList.docs.json | Documented the new loading prop with its default value and description |
| .changeset/calm-hoops-tie.md | Added a minor release changelog entry for loading support |
Comments suppressed due to low confidence (2)
packages/react/src/ActionList/TrailingAction.tsx:25
- Add unit tests to cover the
loadingprop inTrailingAction. For example, assert that whenloadingis true, the spinner is rendered instead of the icon and the button is disabled if that’s part of the contract.
loading?: boolean
packages/react/src/ActionList/TrailingAction.tsx:29
- [nitpick] Consider adding a Storybook story or preview showcasing the
loadingstate ofTrailingAction, so consumers can visually verify spinner behavior and layout.
({as = 'button', icon, label, href = null, className, loading, ...props}, forwardedRef) => {
size-limit report 📦
|
| @@ -44,6 +49,7 @@ export const TrailingAction = forwardRef( | |||
| variant="invisible" | |||
| as={as} | |||
| href={href} | |||
| loading={loading} | |||
There was a problem hiding this comment.
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:
What we might want:
| @@ -44,6 +49,7 @@ export const TrailingAction = forwardRef( | |||
| variant="invisible" | |||
| as={as} | |||
There was a problem hiding this comment.
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!
| /** | ||
| * Specify whether the action is in a loading state | ||
| */ | ||
| loading?: boolean |
There was a problem hiding this comment.
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 😁
Closes 5346
Changelog
New
Changed
Removed
Rollout strategy
Testing & Reviewing
Merge checklist