Skip to content

[BpkButton]Add loading prop for BpkButton#4275

Open
GC Zhu (gc-skyscanner) wants to merge 7 commits intomainfrom
add-loading-prop-for-button
Open

[BpkButton]Add loading prop for BpkButton#4275
GC Zhu (gc-skyscanner) wants to merge 7 commits intomainfrom
add-loading-prop-for-button

Conversation

@gc-skyscanner
Copy link
Contributor

@gc-skyscanner GC Zhu (gc-skyscanner) commented Mar 11, 2026

Summary

  • Add loading prop to BpkButton component
  • When loading is true, the button displays a spinner and is disabled to prevent double-submission
  • Updated SCSS, types, tests, stories, and README documentation
    Figma
image ## Changes
  • BpkButton.tsx — added loading prop handling
  • common-types.tsx — added loading to props interface
  • BpkButton.module.scss — added loading state styles
  • BpkButton-test.tsx / accessibility-test.tsx — added test coverage
  • themeAttributes.ts / themeAttributes-test.ts — updated theme attributes
  • BpkButton.figma.tsx — updated Figma mapping
  • examples.tsx / README.md — added usage examples and documentation

Test plan

  • Run npm run jest -- --testPathPattern bpk-component-button to verify unit tests pass
  • Run npm run lint to verify no lint errors
  • Visually verify loading spinner appears on button in Storybook
  • Verify button is non-interactive while in loading state
  • Verify accessibility: screen readers announce loading state correctly

🤖 Generated with Claude Code

Copilot AI review requested due to automatic review settings March 11, 2026 10:42
@gc-skyscanner GC Zhu (gc-skyscanner) added the minor Non breaking change label Mar 11, 2026
@gc-skyscanner GC Zhu (gc-skyscanner) changed the title Add loading prop for BpkButton [BpkButton]Add loading prop for BpkButton Mar 11, 2026
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds a loading prop to BpkButton (V2) so consumers can show an in-button spinner and prevent interaction during async operations, with supporting theming, docs, examples, and test updates.

Changes:

  • Add loading prop to BpkButtonV2, rendering a spinner, applying aria-busy, and disabling interaction.
  • Update button SCSS mixins and component styles to support loading visuals (including link-type theming).
  • Extend tests (unit + a11y), docs, examples, and Figma mapping; add a new linkThemeAttributes export.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
packages/bpk-mixins/_buttons.scss Refactors hover/pressed token usage into shared mixins and adds loading-disabled visual overrides.
packages/bpk-component-button/src/themeAttributes.ts Adds linkThemeAttributes for theming link loading colour.
packages/bpk-component-button/src/themeAttributes-test.ts Tests new linkThemeAttributes export.
packages/bpk-component-button/src/BpkButtonV2/common-types.tsx Adds loading?: boolean to the public props type.
packages/bpk-component-button/src/BpkButtonV2/accessibility-test.tsx Adds a11y coverage for loading=true across button types and checks aria-busy.
packages/bpk-component-button/src/BpkButtonV2/BpkButton.tsx Implements loading UI (spinner + hidden content) and disables interaction when loading.
packages/bpk-component-button/src/BpkButtonV2/BpkButton.module.scss Adds layout styles for loading spinner/content.
packages/bpk-component-button/src/BpkButtonV2/BpkButton.figma.tsx Exposes loading in the Figma-connected example mapping.
packages/bpk-component-button/src/BpkButtonV2/BpkButton-test.tsx Adds unit tests for loading behaviour and spinner variants.
packages/bpk-component-button/index.ts Re-exports linkThemeAttributes.
packages/bpk-component-button/README.md Documents the loading state usage and link loading theming.
examples/bpk-component-button/examples.tsx Adds Storybook examples showcasing loading state for multiple variants.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

}

&__content--hidden {
visibility: hidden;
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

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

visibility: hidden on the children wrapper will likely remove the button’s text from the accessibility tree in real browsers, which can cause the button to lose its accessible name while loading=true. Consider using a visually-hidden approach that keeps content available to assistive tech (e.g., opacity: 0 / color: transparent) or setting an explicit aria-label/aria-labelledby on the button during loading.

Suggested change
visibility: hidden;
opacity: 0;
color: transparent;

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated in 2adc2e8

@skyscanner-backpack-bot
Copy link

Visit https://backpack.github.io/storybook-prs/4275 to see this build running in a browser.

1 similar comment
@skyscanner-backpack-bot
Copy link

Visit https://backpack.github.io/storybook-prs/4275 to see this build running in a browser.

@skyscanner-backpack-bot
Copy link

skyscanner-backpack-bot bot commented Mar 12, 2026

Warnings
⚠️

Package source files (e.g. packages/package-name/src/Component.js) were updated, but snapshots weren't. Have you checked that the tests still pass?

Browser support

If this is a visual change, make sure you've tested it in multiple browsers.

Generated by 🚫 dangerJS against f205984

@gc-skyscanner GC Zhu (gc-skyscanner) force-pushed the add-loading-prop-for-button branch from 5f004dc to 2adc2e8 Compare March 12, 2026 02:59
@skyscanner-backpack-bot
Copy link

Visit https://backpack.github.io/storybook-prs/4275 to see this build running in a browser.

const isLinkType = type === BUTTON_TYPES.link || type === BUTTON_TYPES.linkOnDark;
const alternate = type === BUTTON_TYPES.linkOnDark;
const shouldUnderline = isLinkType && !iconOnly && !disabled;
const shouldUnderline = isLinkType && !iconOnly && !isDisabled;
Copy link
Contributor Author

@gc-skyscanner GC Zhu (gc-skyscanner) Mar 12, 2026

Choose a reason for hiding this comment

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

Image

: <BpkSpinner type={getSpinnerType(type)} alignToButton />}
</span>
<div className={getCommonClassName('bpk-button__content--hidden')}>
{innerContent}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This content is still required and we need to set its opacity to 0 to maintain the same dimensions as when it’s not in a loading state.
The reason for setting opacity: 0 is referenced here: https://github.com/Skyscanner/backpack/pull/4275#discussion_r2917502180.https://github.com/Skyscanner/backpack/pull/4275#discussion_r2917502180

);

@include utils.bpk-hover {
@include utils.bpk-themeable-property(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

extract it to reduce duplication, since the styling of loading state is same with hover's

@skyscanner-backpack-bot
Copy link

Visit https://backpack.github.io/storybook-prs/4275 to see this build running in a browser.

@gc-skyscanner
Copy link
Contributor Author

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

@skyscanner-backpack-bot
Copy link

Visit https://backpack.github.io/storybook-prs/4275 to see this build running in a browser.

className={classNames}
{...getDataComponentAttribute('Button')}
onClick={onClick}
target={target}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

didn't add aria-busy={loading || undefined} in since it won't hit when it is loading, becasue isDisabled will be set as true

}

&__content--hidden {
opacity: 0;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

? <span className={underlinedClassName}>{children}</span>
: children;

const content = loading ? (
Copy link
Contributor Author

@gc-skyscanner GC Zhu (gc-skyscanner) Mar 12, 2026

Choose a reason for hiding this comment

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

I can imagine that if the PR for leadingIcon and trailingIcon gets merged first, we’ll need to wrap leadingIcon and trailingIcon inside the content container. Otherwise, leadingIcon and trailingIcon will still be visible when the button is in a loading state.

https://github.com/Skyscanner/backpack/pull/4269/changes#diff-2689f3f4f8ca26010db7794aa0cf720fd797f59dc63380171b13a29377e9d2b1

type = BUTTON_TYPES.primary,
...rest
}: Props) => {
const isDisabled = disabled || loading;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When it is loading, in functionality perspective, it should disabled, but the styling is different from the disabled

Image

@skyscanner-backpack-bot
Copy link

Visit https://backpack.github.io/storybook-prs/4275 to see this build running in a browser.

@gc-skyscanner GC Zhu (gc-skyscanner) force-pushed the add-loading-prop-for-button branch from 886205d to 2ab9461 Compare March 12, 2026 07:59
@skyscanner-backpack-bot
Copy link

Visit https://backpack.github.io/storybook-prs/4275 to see this build running in a browser.

@skyscanner-backpack-bot
Copy link

Visit https://backpack.github.io/storybook-prs/4275 to see this build running in a browser.

Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM, can you hold until the icon PR is in and then rebase?

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

Labels

minor Non breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants