Skip to content

[BD-9545][BpkLink] Add bpk-link--explicit for the new bpk-link design. #3768

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
Merged
merged 6 commits into from
Apr 10, 2025
Merged

[BD-9545][BpkLink] Add bpk-link--explicit for the new bpk-link design. #3768

merged 6 commits into from
Apr 10, 2025

Conversation

isteven-shu
Copy link
Contributor

@isteven-shu isteven-shu commented Mar 12, 2025 •
edited
Loading

Jira -- Explicit Link

image\

Description:

  1. update the bpk-link to the new explicit bpk-link style;
  2. add a new boolean prop "implicit" to bpk-link to enable using old design;
  3. add a new "bpk-link--implicit" example and update the propTypes and defaultProps.

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 [KOA-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 minor Non breaking change label Mar 12, 2025
@skyscanner-backpack-bot
Copy link

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

@skyscanner-backpack-bot
Copy link

skyscanner-backpack-bot bot commented Mar 12, 2025 •
edited
Loading

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 f1ea6e5

@skyscanner-backpack-bot
Copy link

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

@isteven-shu isteven-shu changed the title [BD-9545][BpkLink] Add bpk-link--explicit for the new bpk-link design. [BD-9545][BpkLink] Add bpk-link--implicit for the new bpk-link design. Mar 13, 2025
@skyscanner-backpack-bot
Copy link

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

@isteven-shu isteven-shu changed the title [BD-9545][BpkLink] Add bpk-link--implicit for the new bpk-link design. [BD-9545][BpkLink] Add bpk-link--explicit for the new bpk-link design. Mar 13, 2025
@skyscanner-backpack-bot
Copy link

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

@kerrie625
Copy link
Contributor

kerrie625 commented Mar 13, 2025 •
edited
Loading

Test it in the story, hover the link underline disappear then click the link underline display again. The moment of clicking, two lines appeared. Is it expected ？
image

link click

@isteven-shu
Copy link
Contributor Author

Test it in the story, hover the link underline disappear then click the link underline display again. The moment of clicking, two lines appeared. Is it expected ？ image

link click link click

That's definitely a bug. Thanks!

@skyscanner-backpack-bot
Copy link

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

2 similar comments
@skyscanner-backpack-bot
Copy link

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

@skyscanner-backpack-bot
Copy link

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

@isteven-shu isteven-shu added major Breaking change and removed minor Non breaking change labels Mar 13, 2025
@skyscanner-backpack-bot
Copy link

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

@senzg
Copy link
Contributor

senzg commented Mar 13, 2025 •
edited
Loading

Would it be better to set this prop optional like implicit?: boolean, ? I am not sure whether there are other consumers using this module and if it is the case, it seems to create a breaking change? cc @olliecurtis @kerrie625
image

Steven Wang added 2 commits March 13, 2025 17:44
[BD-9545] Underline BPK contribution for total price
d2277e9 
1. add a new boolean prop "explicit" to bpk-link to enable new design;
2. add a new "bpk-link--explicit" classname and corresponding SCSS code for the format.
Add example
ea9b890
@skyscanner-backpack-bot
Copy link

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

@olliecurtis
Copy link
Member

Would it be better to set this prop optional like implicit?: boolean, ? I am not sure whether there are other consumers using this module and if it is the case, it seems to create a breaking change? cc @olliecurtis @kerrie625 image

Provided that we add the declaration to the propTypes section and fill in the defaultProps as per this comment from Kerrie #3768 (comment)

This would make it optional. If we set the type to be implicit?: boolean this would declare that passing any value is optional and could result in a value of undefined which could cause more problems :)

@skyscanner-backpack-bot
Copy link

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

1. Update new explicit bpklink as default;
0178872 
2. Update propTypes and defaultProps;
3. Use `position = abosolute` for underline and `position = relative` for bpkLink to avoid affect on line height.
@skyscanner-backpack-bot
Copy link

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

@isteven-shu isteven-shu requested a review from kerrie625 March 17, 2025 03:34
kerrie625
kerrie625 approved these changes Mar 17, 2025
View reviewed changes
Copy link
Contributor

@kerrie625 kerrie625 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM 🚀 . As this major ,we need merge it wait for next major release

@skyscanner-backpack-bot
Copy link

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

@skyscanner-backpack-bot
Copy link

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

@skyscanner-backpack-bot
Copy link

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

@Faye-Xiao Faye-Xiao merged commit 9fac38f into main Apr 10, 2025
8 checks passed
@Faye-Xiao Faye-Xiao deleted the steven_wang branch April 10, 2025 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kerrie625 kerrie625 kerrie625 approved these changes

@jasonhuang-sky jasonhuang-sky Awaiting requested review from jasonhuang-sky

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 @kerrie625 @senzg @olliecurtis @jasonhuang-sky @Faye-Xiao