[BD-9759][BpkPopover] Add close button beside body #3883

karlahuang · 2025-07-15T08:26:29Z

We found close icon button always be with title together. But sometimes we need the button while no title, so need to handle this case in codes.

Remember to include the following changes:

skyscanner-backpack-bot · 2025-07-15T08:31:42Z

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

skyscanner-backpack-bot · 2025-07-15T08:34:15Z

	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 55f0fd6

examples/bpk-component-popover/examples.js

gert-janvercauteren · 2025-07-16T06:01:11Z

packages/bpk-component-popover/src/BpkPopover.tsx

+                  <div className={bodyClassNames}>
+                    <div>{children}</div>
+                    {!labelAsTitle && closeButtonIcon && (
+                      <BpkCloseButton


In the code above we also define a closebutton, can we ensure both close buttons us the same code (ie. refactor to a const)

yeah, it's the same code, let me do some refactoring. Thank you

…-9759-Add-close-button-beside-BpkPopover-body

skyscanner-backpack-bot · 2025-07-16T06:50:31Z

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

Co-authored-by: tuxiu.luo <359703836@qq.com>

skyscanner-backpack-bot · 2025-07-16T07:21:45Z

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

LuoTuxiu

LGTM

Copilot

Pull Request Overview

This PR implements the ability to display a close button in the popover body when there's no title, addressing the case where a close icon button is needed without a title being present. The change allows for more flexible popover configurations while maintaining existing functionality.

Extracts close button elements into reusable components for better code organization
Adds conditional rendering of close button icon in popover body when labelAsTitle is false
Updates CSS to support flexbox layout for proper positioning of close button beside body content

Reviewed Changes

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

Show a summary per file

File	Description
`BpkPopover.tsx`	Refactors close button rendering logic and adds close button to body when no title is present
`BpkPopover.module.scss`	Adds flexbox styling to popover body for close button positioning
`BpkPopover-test.tsx`	Adds test coverage for new close button functionality and fixes code style issues
`stories.js`	Exports new story example for popover without title
`examples.js`	Implements example component demonstrating popover without title functionality

Copilot · 2025-07-16T10:36:33Z

packages/bpk-component-popover/src/BpkPopover.tsx

@@ -300,7 +295,10 @@ const BpkPopover = ({
                      {label}
                    </span>
                  )}
-                  <div className={bodyClassNames}>{children}</div>
+                  <div className={bodyClassNames}>
+                    <div>{children}</div>


Wrapping children in an anonymous div without any styling or semantic purpose adds unnecessary DOM nesting. Consider removing this wrapper div or adding appropriate CSS classes if styling is needed.

Suggested change

<div>{children}</div>

{children}

🤔 Why do we need a nested DIV here? Any specific consideration?

I just keep the original layout, previous children is also wrapped with div. Maybe because children could be some pure strings

I'm not sure if I find the right place. I noticed the children is a ReactNode prop. I didn't see any wrapped div.
Is this the previous code?

backpack/packages/bpk-component-popover/src/BpkPopover.tsx

Lines 303 to 308 in e55c824

<div className={bodyClassNames}>{children}</div>

{actionText && onAction && (

<div className={getClassName('bpk-popover__action')}>

<BpkButtonLink onClick={onAction}>

{actionText}

</BpkButtonLink>

yeah, I mean this line <div className={bodyClassNames}>{children}</div> , it would keep the layout of children

I closely examined the storybook case and discovered that the reason an inner DIV is necessary is due to the new class name, bpk-popover__body, which uses a flex layout. This layout requires a DIV to wrap the child elements; without it, the layout will not function correctly.

packages/bpk-component-popover/src/BpkPopover.module.scss

skyscanner-backpack-bot · 2025-07-17T11:50:06Z

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

This reverts commit 84a880a.

…3894) This reverts commit 84a880a.

…3883)" (…" This reverts commit 12aa0ec.

* main: [QUAR-991] [BpkInsetBannerSponsored] Add closeOnScrimClick prop to BpkBottomSheet (#3891) Revert "[BD-9759][BpkPopover] Add close button beside body (#3883)" (#3894) optimize cardlist style (#3893) [CLOV-402][BpkCardList] Fix cardlist initially shown cards change issue (#3892)

BD-9759-Add-close-button-beside-BpkPopover-body

0333ed8

karlahuang changed the title ~~[BD-9759] Add close button beside BpkPopover body~~ [BD-9759][BpkPopover] Add close button beside body Jul 15, 2025

karlahuang added the patch Patch production bug label Jul 15, 2025

karlahuang requested a review from gert-janvercauteren July 15, 2025 10:27

gert-janvercauteren reviewed Jul 16, 2025

View reviewed changes

karlahuang added 2 commits July 16, 2025 14:45

rafactoring

23b4984

Merge branch 'main' of https://github.com/Skyscanner/backpack into BD…

a25ced8

…-9759-Add-close-button-beside-BpkPopover-body

Update examples/bpk-component-popover/examples.js

7f08843

Co-authored-by: tuxiu.luo <359703836@qq.com>

LuoTuxiu approved these changes Jul 16, 2025

View reviewed changes

Faye-Xiao requested a review from Copilot July 16, 2025 10:35

Copilot AI reviewed Jul 16, 2025

View reviewed changes

Faye-Xiao added minor Non breaking change and removed patch Patch production bug labels Jul 16, 2025

Faye-Xiao approved these changes Jul 17, 2025

View reviewed changes

Merge branch 'main' into BD-9759-Add-close-button-beside-BpkPopover-body

55f0fd6

Faye-Xiao merged commit 84a880a into main Jul 17, 2025
10 checks passed

Faye-Xiao deleted the BD-9759-Add-close-button-beside-BpkPopover-body branch July 17, 2025 11:56

Supremeyh added a commit that referenced this pull request Jul 28, 2025

Revert "[BD-9759][BpkPopover] Add close button beside body (#3883)"

29f1f12

This reverts commit 84a880a.

Supremeyh mentioned this pull request Jul 28, 2025

Revert "[BD-9759][BpkPopover] Add close button beside body" #3894

Merged

Supremeyh added a commit that referenced this pull request Jul 28, 2025

Revert "[BD-9759][BpkPopover] Add close button beside body (#3883)" (#…

12aa0ec

…3894) This reverts commit 84a880a.

karlahuang added a commit that referenced this pull request Jul 28, 2025

Revert "Revert "[BD-9759][BpkPopover] Add close button beside body (#…

e70f09a

…3883)" (…" This reverts commit 12aa0ec.

	<div className={bodyClassNames}>{children}</div>
	{actionText && onAction && (
	<div className={getClassName('bpk-popover__action')}>
	<BpkButtonLink onClick={onAction}>
	{actionText}
	</BpkButtonLink>

[BD-9759][BpkPopover] Add close button beside body #3883

[BD-9759][BpkPopover] Add close button beside body #3883

Uh oh!

Conversation

karlahuang commented Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skyscanner-backpack-bot bot commented Jul 15, 2025

Uh oh!

skyscanner-backpack-bot bot commented Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Browser support

Uh oh!

Uh oh!

gert-janvercauteren Jul 16, 2025

Choose a reason for hiding this comment

Uh oh!

karlahuang Jul 16, 2025

Choose a reason for hiding this comment

Uh oh!

skyscanner-backpack-bot bot commented Jul 16, 2025

Uh oh!

skyscanner-backpack-bot bot commented Jul 16, 2025

Uh oh!

LuoTuxiu left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jul 16, 2025

Choose a reason for hiding this comment

Uh oh!

Faye-Xiao Jul 16, 2025

Choose a reason for hiding this comment

Uh oh!

karlahuang Jul 17, 2025

Choose a reason for hiding this comment

Uh oh!

Faye-Xiao Jul 17, 2025

Choose a reason for hiding this comment

Uh oh!

karlahuang Jul 17, 2025

Choose a reason for hiding this comment

Uh oh!

Faye-Xiao Jul 17, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

skyscanner-backpack-bot bot commented Jul 17, 2025

Uh oh!

Uh oh!

Uh oh!

karlahuang commented Jul 15, 2025 •

edited

Loading

skyscanner-backpack-bot bot commented Jul 15, 2025 •

edited

Loading