Skip to content

[NOJIRA][BpkModal]fix closeText style of BpkModal in dark mode #3919

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 1 commit into from
Aug 25, 2025
Merged

[NOJIRA][BpkModal]fix closeText style of BpkModal in dark mode #3919

merged 1 commit into from
Aug 25, 2025

Conversation

GCpigsic
Copy link
Contributor

@GCpigsic GCpigsic commented Aug 25, 2025 •
edited
Loading

When we wrongly set the onDark prop to BpkButtonLink before(original PR), but actually, we don't have onDark in BpkButtonLink, but alternate instead

backpack/packages/bpk-component-link/src/BpkButtonLink.js

Line 36 in 9d698ea

alternate: boolean,

this would also cause an warning in dev when comsumer uses BpkModal
image

Screenshot

before after
image image

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

@Copilot Copilot AI review requested due to automatic review settings August 25, 2025 06:43
@GCpigsic GCpigsic added the patch Patch production bug label Aug 25, 2025
@skyscanner-backpack-bot
Copy link

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

@skyscanner-backpack-bot
Copy link

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 0e0bb39

GCpigsic
GCpigsic commented Aug 25, 2025
View reviewed changes
packages/bpk-component-modal/src/BpkModalInner.tsx
@@ -154,7 +154,7 @@ const BpkModalInner = ({
closeText ? (
<BpkButtonLink
onClick={onClose}
onDark={modalStyle === MODAL_STYLING.surfaceContrast}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we wrongly passed onDark to BpkButtonLink in https://github.com/Skyscanner/backpack/pull/3447/files#diff-df999835b1d554fe1ab9384a45253714a087966c76a54f3e00e1869ca698430dR162
But it should be alternate

backpack/packages/bpk-component-link/src/BpkButtonLink.js

Line 36 in 9d698ea

alternate: boolean,

GCpigsic
GCpigsic commented Aug 25, 2025
View reviewed changes
examples/bpk-component-modal/examples.tsx
@@ -160,7 +160,12 @@ const ContrastExample = (isOpen: boolean) => (
);

const WideExample = (isOpen: boolean) => (
<ModalContainer title="Modal title" closeLabel="Close modal" isOpen={isOpen} wide>
<ModalContainer
Copy link
Contributor Author

Choose a reason for hiding this comment

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

format the file

GCpigsic
GCpigsic commented Aug 25, 2025
View reviewed changes
examples/bpk-component-modal/examples.tsx
@@ -177,6 +182,17 @@ const CloseButtonTextExample = (isOpen: boolean) => (
</ModalContainer>
);

const ContrastWithCloseButtonTextExample = (isOpen: boolean) => (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

add a storybook for contrast with closeButtonText

@skyscanner-backpack-bot
Copy link

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

@GCpigsic
Copy link
Contributor Author

percy failed caused by unstable percy, run sucessfully in local
image

Copilot
Copilot AI reviewed Aug 25, 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.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@IrinaWei
Copy link
Contributor

LGTM

Faye-Xiao
Faye-Xiao approved these changes Aug 25, 2025
View reviewed changes
Copy link
Contributor

@Faye-Xiao Faye-Xiao left a comment

Choose a reason for hiding this comment

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

LGTM

@GCpigsic GCpigsic merged commit d581bb7 into main Aug 25, 2025
16 of 17 checks passed
@GCpigsic GCpigsic deleted the fix-contrast-model-close-button branch August 25, 2025 09:05
gert-janvercauteren added a commit that referenced this pull request Aug 26, 2025
@gert-janvercauteren
Merge branch 'main' into feature/mcp-code-connect
4d5fd0f 
* main:
  Bump actions/download-artifact in the artifacts-actions group (#3907)
  Bump actions/checkout from 4 to 5 (#3913)
  Fix closeText style of BpkModal in dark mode (#3919)
  Bump the babel group across 1 directory with 5 updates (#3917)
  Bump @babel/runtime in the npm_and_yarn group across 1 directory (#3884)
  Bump actions/cache from 4.2.3 to 4.2.4 (#3905)
  Bump webpack from 5.99.8 to 5.101.2 (#3916)
  QUAR-1046 Fix CTA Button Alignment, Logo Sizing, and Logo Vertical Alignment in Inset Banner (#3912)
  chore: removed duplicate aria-label (#3911)
  [CAPY-1594][BpkNavigationTabGroup/BpkBubble] Create and integrate 'new' bubble tooltip in navigation tab bar (#3909)
  [CLOV-381][BpkBreakpoint] update bpk breakpoint readme (#3910)
  [CYB-3904][BpkGraphicPromo] Render wrapper as semantic anchor tag to improve SEO (#3904)
  [CLOVER-532][BpkText] Add BpkText color prop with leverage css classname (#3900)
  fix: A11y and icon issue for inset banner sponsored (#3901)

# Conflicts:
#	packages/bpk-component-text/README.md
#	packages/bpk-component-text/src/BpkText-test.tsx
#	packages/bpk-component-text/src/BpkText.tsx
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

Copilot code review Copilot Copilot left review comments

@Faye-Xiao Faye-Xiao Faye-Xiao approved these changes

@IrinaWei IrinaWei IrinaWei approved these changes

Assignees
No one assigned
Labels
patch Patch production bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@GCpigsic @IrinaWei @Faye-Xiao