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-497] [BpkInsetBanner] Add CTA button & popover #3732

Merged
merged 10 commits into from
Feb 7, 2025

Conversation

kirstybryce
Copy link
Contributor

@kirstybryce kirstybryce commented Jan 31, 2025

JIRA: https://skyscanner.atlassian.net/browse/QUAR-497

When the BpkInsetBanner component was created last year we added the CTA object prop accepting a text element. Now we need to add the CTA button and popover functionality so that we can add required legal info in the adverts using the inset banner.

Props added to CallToAction object prop:

  • popoverMessage: message that will be displayed inside the popover dialog when the CTA button is clicked.
  • popoverPlacement: where the popover dialog should appear when CTA button is clicked (ex. 'top', 'bottom', etc).
  • buttonCloseLabel: text to display in the close button of the popover dialog.
  • buttonA11yLabel: accessibility aria label for the CTA button.
  • popverLabel: label of the popover dialog, and if labelTitle is true, title to display at the top of the popover dialog.
  • popoverId: id of the popover dialog.
  • labelTitle: if true, displays popoverLabel as popover title.
  • closeBtnIcon: if true, displays close icon instead of buttonCloseLabel in popover close button.
Without CTA Popover With CTA Popover (onLight variant) With CTA Popover (onDark variant)
Screenshot 2025-02-03 at 16 03 56 Screenshot 2025-02-03 at 16 03 46 Screenshot 2025-02-05 at 18 12 11

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

@skyscanner-backpack-bot
Copy link

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

@skyscanner-backpack-bot
Copy link

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

@kirstybryce kirstybryce changed the title [QUAR-497] Update BpkInsetBanner to include CTA btn [QUAR-497] [BpkInsetBanner] Add CTA button & popover Feb 3, 2025
@skyscanner-backpack-bot
Copy link

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

@kirstybryce kirstybryce added the minor Non breaking change label Feb 3, 2025
@olliecurtis olliecurtis added major Breaking change and removed minor Non breaking change labels Feb 5, 2025
@olliecurtis olliecurtis added minor Non breaking change and removed major Breaking change labels Feb 5, 2025
@skyscanner-backpack-bot
Copy link

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

@skyscanner-backpack-bot
Copy link

skyscanner-backpack-bot bot commented Feb 5, 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?

⚠️

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 b5da5fb

@skyscanner-backpack-bot
Copy link

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

@skyscanner-backpack-bot
Copy link

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

popoverPlacement?: Placement;
buttonCloseLabel?: string;
buttonA11yLabel?: string;
popverLabel?: string;
Copy link

@hir06 hir06 Feb 6, 2025

Choose a reason for hiding this comment

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

would it be nice to have aria-labelledby here too?

Copy link

Choose a reason for hiding this comment

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

I mean for popover

Copy link
Contributor Author

Choose a reason for hiding this comment

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

buttonA11yLabel is used to pass a value to aria-label on the button so I think that's sufficient, as aria-labelledby requires an id to be passed to refer to an element to be used as the label of the object. But with the way backpack components work in the consumer, the backpack component is abstracted and we don't see the IDs inside the backpack component. In this case I think having buttonA11yLabel is preferable :)

Copy link

Choose a reason for hiding this comment

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

I see so here the button would be info icon and the label typically could be description of popover?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Button would be info icon, and if CTA text provided then text + info icon is in the button, so what would be read out by the screen reader is either "aria-label text", button or "aria-label text", button, "cta text"

@@ -52,6 +52,28 @@ describe('BpkInsetBanner', () => {
expect(getByText('Sponsored')).toBeInTheDocument();
});

it('should render call to action popover text if provided', () => {
Copy link

Choose a reason for hiding this comment

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

should we also test that the popover can be closed properly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The popover close functionality is tested in the popover component, so I think there's enough coverage for that there, and in inset banner we just want to test the popover message prop is passed correctly and shown in the popover dialog :)

Copy link

@MaxHun MaxHun left a comment

Choose a reason for hiding this comment

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

left 1 comment, rest lgtm

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

@kirstybryce kirstybryce marked this pull request as ready for review February 7, 2025 11:41
@skyscanner-backpack-bot
Copy link

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

e.stopPropagation();
e.preventDefault();
}}
aria-label={callToAction?.buttonA11yLabel}
Copy link
Member

Choose a reason for hiding this comment

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

Will need to check this providing this alongside the label, as looking at the BpkPopover this might compete with label which also sets an A11y label for the popover based on the prop

Copy link
Member

Choose a reason for hiding this comment

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

Just testing this and this property is having no effect to a screenreader as the aria-labelledby inside the popover is being prioritsed over it and using the label (which is the popover title) to be the aria for the user :) so this can be omitted

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I'll remove it :)

@skyscanner-backpack-bot
Copy link

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

@olliecurtis olliecurtis merged commit ecef296 into main Feb 7, 2025
8 checks passed
@olliecurtis olliecurtis deleted the QUAR-497_Update-BpkInsetBanner-to-include-CTA-btn branch February 7, 2025 15:05
@kirstybryce kirstybryce restored the QUAR-497_Update-BpkInsetBanner-to-include-CTA-btn branch February 17, 2025 14:21
@kirstybryce kirstybryce deleted the QUAR-497_Update-BpkInsetBanner-to-include-CTA-btn branch February 17, 2025 14:21
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