Skip to content
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.

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

[QUAR-642] [BpkCardWrapper] Add more info button and body section to card wrapper #3730

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

kirstybryce
Copy link
Contributor

@kirstybryce kirstybryce commented Jan 29, 2025

https://skyscanner.atlassian.net/browse/QUAR-642

Figma designs: https://www.figma.com/design/22unPHkDCzqJkd9rKzTiGY/T%26Cs-Ads?node-id=79-11694&t=MTrz7gFIDvIgP4k2-4

For the inline ads, we need to have a dropdown box for T&C in the ad card wrapper. We're currently use a custom component to do this for desktop ads, but we need to add it to mobile ads as well. Instead of duplicating the custom wrapper into the mobile web ad components, we want to use the BpkCardWrapper component instead. To use it though we need to add the more info button and dropdown body element.

More Info

Before Screenshot

Less Info

After Screenshot

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

@kirstybryce kirstybryce changed the title [QUAR-642] Add more info button and body section to card wrapper [QUAR-642] [BpkCardWrapper] Add more info button and body section to card wrapper Jan 29, 2025
Copy link

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

Copy link

github-actions bot commented Jan 29, 2025

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 1b29324

Copy link

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

Copy link

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

Copy link

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

Copy link

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

@olliecurtis olliecurtis added the minor Non breaking change label Jan 30, 2025
@skyscanner-backpack-bot
Copy link

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

@skyscanner-backpack-bot
Copy link

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

@skyscanner-backpack-bot
Copy link

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

@skyscanner-backpack-bot
Copy link

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

@sharada1022 sharada1022 marked this pull request as ready for review February 18, 2025 08:15
@skyscanner-backpack-bot
Copy link

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

@skyscanner-backpack-bot
Copy link

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

@hir06
Copy link

hir06 commented Feb 18, 2025

Can you add screenshots for the visual changes in PR description?


.bpk-card-wrapper {
padding: 0.1rem;
Copy link

Choose a reason for hiding this comment

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

why give fixed number here? will it not impact the visual test cases of other consumers already using bpk-card-wrapper?

Choose a reason for hiding this comment

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

yes, you are right. I’ve updated the padding to use the predefined spacing token.

@skyscanner-backpack-bot
Copy link

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

1 similar comment
@skyscanner-backpack-bot
Copy link

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

Copy link

@hir06 hir06 left a comment

Choose a reason for hiding this comment

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

LGTM but please check for build failures and warnings and visual tests for consumer of the component

Copy link

@hir06 hir06 left a comment

Choose a reason for hiding this comment

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

I see some design conflict with expand version of the wrapper for eg. rounded corner for the body details are not present on desktop or mobile.

@skyscanner-backpack-bot
Copy link

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

@sharada1022
Copy link

LGTM but please check for build failures and warnings and visual tests for consumer of the component

All the checks has been passed and visual test is picked up by Percy to run on CI.

@sharada1022
Copy link

I see some design conflict with expand version of the wrapper for eg. rounded corner for the body details are not present on desktop or mobile.

I checked with the design team, and they confirmed that we should maintain the full card look with rounded corners as long as the transition is smooth.

@skyscanner-backpack-bot
Copy link

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

@skyscanner-backpack-bot
Copy link

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

Copy link
Contributor Author

@kirstybryce kirstybryce left a comment

Choose a reason for hiding this comment

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

Just a small UI change needed, otherwise LGTM :)

@skyscanner-backpack-bot
Copy link

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

@skyscanner-backpack-bot
Copy link

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

Copy link

@hir06 hir06 left a comment

Choose a reason for hiding this comment

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

LGTM please check the pipeline failures.

border-radius: 0 0 tokens.$bpk-border-radius-md tokens.$bpk-border-radius-md;
transition: border-radius 0.25s ease-in-out;
border-radius: tokens.$bpk-border-radius-md;
background-color: tokens.$bpk-private-info-banner-default-day;
Copy link
Member

Choose a reason for hiding this comment

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

This should not use a private component token however the same colour exists as

Suggested change
background-color: tokens.$bpk-private-info-banner-default-day;
background-color: tokens.$bpk-canvas-contrast-day;

display: flex;
padding: tokens.bpk-spacing-base();
flex-direction: column;
background-color: tokens.$bpk-private-info-banner-default-day;
Copy link
Member

Choose a reason for hiding this comment

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

This should not use a private component token however the same colour exists as

Suggested change
background-color: tokens.$bpk-private-info-banner-default-day;
background-color: tokens.$bpk-canvas-contrast-day;


&--link-text {
width: max-content;
color: tokens.$bpk-private-button-link-normal-foreground-day;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
color: tokens.$bpk-private-button-link-normal-foreground-day;
color: tokens.$bpk-text-link-day;

@@ -31,27 +37,127 @@ type Props = {
className?: string | null;
backgroundColor: string;
header: ReactNode;
body?: {
Copy link
Member

Choose a reason for hiding this comment

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

body as a name I think might be confusing to consumers, maybe we could call this expandContent so its clear where this would be.

wdyt?

card,
className = null,
header,
}: Props) => {
const classNames = getClassName('bpk-card-wrapper', className);

return (
<CardContext.Provider value={{ elevated: false }}>
const [isBodyOpen, setIsBodyOpen] = useState(false);
Copy link
Member

Choose a reason for hiding this comment

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

For this I would call this

Suggested change
const [isBodyOpen, setIsBodyOpen] = useState(false);
const [isExpanded, setIsExpanded] = useState(false);

return (
<CardContext.Provider value={{ elevated: false }}>
const [isBodyOpen, setIsBodyOpen] = useState(false);
const bodyRef = useRef<HTMLDivElement>(null);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const bodyRef = useRef<HTMLDivElement>(null);
const expandRef = useRef<HTMLDivElement>(null);

</div>
);

const headerWithBodyDiv = body && (
Copy link
Member

Choose a reason for hiding this comment

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

I would call this expandableHeader

@@ -69,4 +79,56 @@
opacity: 1;
}
}

&--body {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
&--body {
&--expand {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor Non breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants