8000 [BD-9793][BpkButton] Reimplement BpkButton(type=link) using gradient background. by isteven-shu · Pull Request #3823 · Skyscanner/backpack · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

[BD-9793][BpkButton] Reimplement BpkButton(type=link) using gradient background. #3823

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

Jump to bottom
Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

[BD-9793][BpkButton] Reimplement BpkButton(type=link) using gradient background. #3823

wants to merge 2 commits into from

Conversation

isteven-shu
Copy link
Contributor
@isteven-shu isteven-shu commented May 12, 2025 •
edited
Loading

JIRA link
Reimplement BpkButton(type=link) using gradient backgound.

Remember to include the following changes:

  • Ensure the PR title includes the name of the component you are changing so it's clear in the release notes for consumers of the changes in the version e.g [Clover-123][BpkButton] Updating the colour
  • README.md (If you have created a new component)
  • Component README.md
  • Tests
  • Accessibility tests
    • The following checks were performed:
      • Ability to navigate using a keyboard only
      • Zoom functionality (Deque University explanation):
        • The page SHOULD be functional AND readable when only the text is magnified to 200% of its initial size
        • Pages must reflow as zoom increases up to 400% so that content continues to be presented in only one column i.e. Content MUST NOT require scrolling in two directions (both vertically and horizontally)
      • Ability to navigate using a screen reader only
  • Storybook examples created/updated
  • For breaking changes or deprecating components/properties, migration guides added to the description of the PR. If the guide has large changes, consider creating a new Markdown page inside the component's docs folder and link it here

@isteven-shu isteven-shu added the major Breaking change label May 12, 2025
@skyscanner-backpack-bot
Copy link

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

@skyscanner-backpack-bot
Copy link

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

@isteven-shu isteven-shu changed the title [BD-9793][BpkButton] Reimplement BpkButton(type=link) using gradient backgound. [BD-9793][BpkButton] Reimplement BpkButton(type=link) using gradient background. May 12, 2025
@skyscanner-backpack-bot
Copy link
skyscanner-backpack-bot bot commented May 12, 2025 •
edited
Loading
Warnings
⚠️

Package source files (e.g. packages/package-name/src/Component.tsx) were updated, but type files weren't. Have you checked that no types have changed?

Browser support

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

Generated by 🚫 dangerJS against 6a5f735

@giriawu
Copy link
Contributor
giriawu commented May 12, 2025

Here are some issue in the storybook:
image
In the fifth example, it should be a combination button with text and an icon. I'm not entirely sure, but my understanding of the expected behavior is:

  1. The underline shouldn't be longer than the text (currently there's visible spacing between the text and the button)?
  2. The underline animation should trigger when the user hovers over the entire button?
  3. Should we also add implicit props like in BpkButtonLink?

@isteven-shu
Copy link
Contributor Author
isteven-shu commented May 12, 2025 •
edited
Loading

Here are some issue in the storybook: image In the fifth example, it should be a combination button with text and an icon. I'm not entirely sure, but my understanding of the expected behavior is:

  1. The underline shouldn't be longer than the text (currently there's visible spacing between the text and the button)?
  2. The underline animation should trigger when the user hovers over the entire button?
  3. Should we also add implicit props like in BpkButtonLink?

Thanks for the catch. The code has been updated.

@skyscanner-backpack-bot
Copy link

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

jasonhuang-sky
jasonhuang-sky reviewed May 12, 2025
View reviewed changes
@skyscanner-backpack-bot
Copy link

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

Supremeyh
Supremeyh reviewed May 15, 2025
View reviewed changes
packages/bpk-component-button/src/BpkButtonV2/BpkButton.tsx
if (typeof child === 'string') {
const match = child.match(/^(\s*)(.*?)(\s*)$/);
const leading = match?.[1] ?? '';
const text = match?.[2] ?? '';
Copy link
Contributor

Choose a reason for hiding this comment

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

We need consider some edge case to make it more robust, for text part, for example if the children is like <Icon />Button<Icon /> Link, it would underline both text Button and Link, which might not be expected

senzg
senzg reviewed May 15, 2025
View reviewed changes
packages/bpk-component-button/src/BpkButtonV2/BpkButton.tsx
<>
{leading.length > 0 && <span>{leading}</span>}
<span className={getCommonClassName(`bpk-button--${type}__text`)}>{text}</span>
{trailing.length > 0 && <span>{trailing}</span>}
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if it's a possible bug: if a user intentionally need blank space between strings and icons, such as this in storybook:

<Wrapped 'Button clicked')} {...rest}>
        <BaggageCheckedIcon />   blankAhead    Button   blankBehind       <AlignedLargeLongArrowRightIcon />
</Wrapped>

We get this, a blank space only occupies one character
image

@Faye-Xiao Faye-Xiao requested a review from Copilot May 28, 2025 06:42
Copilot
Copilot AI reviewed May 28, 2025
View reviewed changes
Copy link
Contributor
@Copilot 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

Reimplements link-style buttons to use a gradient underline by:

  • Swapping low-contrast em padding in bpk-link-underlined for a one-pixel rem variable
  • Refactoring bpk-button--link and bpk-button--link-on-dark mixins to use new __text spans instead of ::after
  • Updating BpkButtonV2 to wrap string children in <span>s for gradient styling, and updating affected snapshots

Reviewed Changes

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

Show a summary per file
File Description
packages/bpk-mixins/_typography.scss Changed padding-bottom: 0.03em to $bpk-one-pixel-rem
packages/bpk-mixins/_buttons.scss Replaced ::after underline with __text span approach
packages/bpk-component-floating-notification/...snap.tsx Updated snapshot to include the new __text wrapper
packages/bpk-component-button/src/BpkButtonV2/snapshots/*.snap.tsx Updated snapshots to match new text-wrapping spans
packages/bpk-component-button/src/BpkButtonV2/BpkButton.tsx Added logic to split string children into <span> elements

@Faye-Xiao
Copy link
Contributor

Hi @isteven-shu Thank you for your contribution!

I have finished reviewing your implementation and had a discussion with our team regarding this style change. Your solution addresses all the cases outlined in the visual design and also handles some edge cases well. Great job! 👍

However, there are a few questions we need to highlight and consider:

  • First and foremost, as we learned from the recent issue (ILD: INC-2041: Falcon blocked for backpack and global component updates due to styling issues), we expect this feature to provide backward compatibility and minimize risks to existing features. From the perspective of a public UI library, enforcing a visual change is acceptable; however, we have noticed that many consumers are very anxious due to new style(underline style) of BpkLink.

  • Regarding the solution, the Children Analysis approach looks good to me. I noticed you introduced regex checks for edge cases, which shows thorough evaluation. However, if I understand correctly, the icons below will not be underlined (as the below screenshot shows) when the button content consists of nested fragments or custom JSX. I'm not sure if this aligns with the designer's expectations, so I’ve raised this question with our designer for confirmation.

image

Additionally, we observed an overlap between BpkButton (type = link) and BpkButtonLink, which could be confusing. Therefore, we're discussing this with our designer. I want to ensure your work continues smoothly, but as your modifications are involved, I believe it’s important to sync up on this with you. There should be no impact at this moment. I will keep you updated once we make a change that may affect your work.

Thank you again!

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

@jasonhuang-sky jasonhuang-sky jasonhuang-sky left review comments

@Supremeyh Supremeyh Supremeyh left review comments

@senzg senzg senzg left review comments

Copilot code review Copilot Copilot left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
major Breaking change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@isteven-shu @giriawu @Faye-Xiao @jasonhuang-sky @Supremeyh @senzg
0