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

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

Supremeyh merged 1 commit into main from revert-3883-BD-9759-Add-close-button-beside-BpkPopover-body

Jul 28, 2025

Contributor

Supremeyh commented Jul 28, 2025

Reverts #3883


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

29f1f12

This reverts commit 84a880a.

Supremeyh added patch minor and removed patch labels

karlahuang approved these changes

View reviewed changes

Supremeyh marked this pull request as ready for review

July 28, 2025 08:03

Copilot AI review requested due to automatic review settings

July 28, 2025 08:03

Copilot AI reviewed

View reviewed changes

Contributor

Copilot AI left a comment

Pull Request Overview

This PR reverts a previous change that added a close button beside the body content in the BpkPopover component. The revert removes the functionality that allowed placing close buttons within the popover body area and restores the original layout structure.

Removes close button elements from being rendered beside the body content
Reverts CSS changes that modified the body layout to use flexbox
Removes associated test cases and example components for the reverted functionality

Reviewed Changes

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

Show a summary per file

File	Description
packages/bpk-component-popover/src/BpkPopover.tsx	Reverts popover component logic to remove close button from body area and inline close button elements
packages/bpk-component-popover/src/BpkPopover.module.scss	Removes flexbox styling from popover body that was used for close button positioning
packages/bpk-component-popover/src/BpkPopover-test.tsx	Removes test case for close button without title and fixes code style issues
examples/bpk-component-popover/stories.js	Removes story export for WithNoTitle example
examples/bpk-component-popover/examples.js	Removes WithNoTitleExample component and fixes formatting

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

-                  'bpk-popover__body',
-                  padded && 'bpk-popover__body--padded',
-                );
+                const bodyClassNames = getClassName(padded && 'bpk-popover__body--padded');

Preview

Copilot AI Jul 28, 2025

The getClassName call is missing the base 'bpk-popover__body' class name. This should be getClassName('bpk-popover__body', padded && 'bpk-popover__body--padded') to ensure the base body class is always applied.

Suggested change

      
              const bodyClassNames = getClassName(padded && 'bpk-popover__body--padded');
          
              const bodyClassNames = getClassName('bpk-popover__body', padded && 'bpk-popover__body--padded');

Copilot uses AI. Check for mistakes.

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

@@ @@ -26,10 +26,10 @@ describe('BpkPopover', () => { @@
                 beforeEach(() => {
                   onCloseSpy = jest.fn();
-                });
+                })

Preview

Copilot AI Jul 28, 2025

Missing semicolon after the closing brace. Should be }); to maintain consistent syntax.

Suggested change

Copilot uses AI. Check for mistakes.

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

                 it('should render correctly', () => {
-                  const target = <button type="button">My target</button>;
+                  const target = (<button type="button">My target</button>);

Preview

Copilot AI Jul 28, 2025

[nitpick] Unnecessary parentheses around JSX element. Should be const target = <button type="button">My target</button>; for consistency with React conventions.

Suggested change

      
                const target = (<button type="button">My target</button>);
          
                const target = <button type="button">My target</button>;

Copilot uses AI. Check for mistakes.

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

@@ @@ -66,12 +66,12 @@ describe('BpkPopover', () => { @@
                         My popover content
                       </BpkPopover>,
                     );
-                  });
+                  })

Preview

Copilot AI Jul 28, 2025

Missing semicolon after the closing brace. Should be }); to maintain consistent syntax.

Suggested change

Copilot uses AI. Check for mistakes.

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

                   expect(screen.queryByRole('My popover content')).not.toBeInTheDocument();
                 });
                 it('should render without an arrow', async () => {
-                  const target = <button type="button">My target</button>;
+                  const target = (<button type="button">My target</button>);

Preview

Copilot AI Jul 28, 2025

[nitpick] Unnecessary parentheses around JSX element. Should be const target = <button type="button">My target</button>; for consistency with React conventions.

Suggested change

      
                const target = (<button type="button">My target</button>);
          
                const target = <button type="button">My target</button>;

Copilot uses AI. Check for mistakes.

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

@@ @@ -87,7 +87,7 @@ describe('BpkPopover', () => { @@
                   );
                   await waitFor(async () => {
                     expect(screen.getByText('My popover content')).toBeVisible();
-                  });
+                  })

Preview

Copilot AI Jul 28, 2025

Missing semicolon after the closing brace. Should be }); to maintain consistent syntax.

Suggested change

Copilot uses AI. Check for mistakes.

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

-                  expect(screen.getByRole('button', { name: /Close/i })).toHaveAttribute(
-                    'tabindex',
-                  );
+                  expect(screen.getByRole('button', { name: /Close/i, })).toHaveAttribute('tabindex');

Preview

Copilot AI Jul 28, 2025

Trailing comma in object literal should be removed. Should be { name: /Close/i } without the trailing comma.

Suggested change

      
                expect(screen.getByRole('button', { name: /Close/i, })).toHaveAttribute('tabindex');
          
                expect(screen.getByRole('button', { name: /Close/i })).toHaveAttribute('tabindex');

Copilot uses AI. Check for mistakes.

skyscanner-backpack-bot bot commented Jul 28, 2025

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

skyscanner-backpack-bot bot commented Jul 28, 2025

	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 29f1f12

kerrie625 approved these changes

View reviewed changes

Supremeyh merged commit 12aa0ec into main

12 of 13 checks passed

Supremeyh deleted the revert-3883-BD-9759-Add-close-button-beside-BpkPopover-body branch

July 28, 2025 08:16

karlahuang mentioned this pull request

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

Closed

12 tasks

gert-janvercauteren added a commit that referenced this pull request


          Merge branch 'main' into feature/mcp-code-connect

0eccc7b

* 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)

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

Labels

minor