Skip to content

[Brand Refactor] Fixes CTABanner - #1362

Merged
danielguillan merged 14 commits into
mainfrom
danielguillan/brand-refactor-qa-cta-banner
Jul 16, 2026
Merged

[Brand Refactor] Fixes CTABanner#1362
danielguillan merged 14 commits into
mainfrom
danielguillan/brand-refactor-qa-cta-banner

Conversation

@danielguillan

@danielguillan danielguillan commented Jun 3, 2026

Copy link
Copy Markdown
Collaborator

Summary

Resolves https://github.com/orgs/github/projects/23995/views/1?pane=issue&itemId=185434843
Resolves https://github.com/orgs/github/projects/23995/views/1?pane=issue&itemId=159782142

Addresses 2 different items of QA feedback from the board.

Towards https://github.com/github/brand-marketing-design/issues/2606

List of notable changes:

  • Restores rounded corners to CTABanner when grid lines are not enabled.
  • Preserves square CTABanner edges when grid lines are enabled so gridline borders stay aligned.
  • Upadte gap inside CTABanner content on large viewports to match designs.
  • Improves ButtonGroup so custom class names are forwarded alongside its default styles.

What should reviewers focus on?

  • Verify CTABanner rounded corners match the expected design in default, balanced, and minimal variants.
  • Verify gridline variants retain square edges and gridline alignment across desktop, tablet, and mobile.
  • Verify CTABanner.ButtonGroup keeps the expected spacing without dropping ButtonGroup default styles.

Steps to test:

  1. Open the CTABanner stories in the Storybook preview.
  2. Verify the default, playground, and variants stories preserve the expected rounded corners.
  3. Verify the WithGridLines story keeps square edges and aligned gridline borders.
  4. Verify stories with buttons still render the expected button group spacing and styles.

Contributor checklist:

  • All new and existing CI checks pass
  • Tests prove that the feature works and covers both happy and unhappy paths
  • Any drop in coverage, breaking changes or regressions have been documented above
  • UI Changes contain new visual snapshots (generated by adding update snapshots label to the PR)
  • All developer debugging and non-functional logging has been removed
  • Related issues have been referenced in the PR description

Reviewer checklist:

  • Check that pull request and proposed changes adhere to our contribution guidelines and code of conduct
  • Check that tests prove the feature works and covers both happy and unhappy paths
  • Check that there aren't other open Pull Requests for the same update/change

Screenshots:

Before After
image image

@changeset-bot

changeset-bot Bot commented Jun 3, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 3fac55e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 9 packages
Name Type
@primer/react-brand Patch
@primer/brand-docs Patch
@primer/brand-css Patch
@primer/brand-primitives Patch
@primer/brand-e2e Patch
@primer/brand-fonts Patch
@primer/brand-mcp Patch
@primer/brand-config Patch
@primer/brand-storybook Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions

github-actions Bot commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

🟢 No design token changes found

@github-actions

github-actions Bot commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

🟢 Bundle size report

CheckMainBranchChange
UMD — full bundle (JS)101.34 kB101.47 kB⬆️ +132 B (+0.1%)
UMD — full bundle (CSS)66.91 kB66.92 kB⬆️ +9 B (+0.0%)
ESM — full bundle (JS + CSS)1.51 MB1.51 MB⬆️ +206 B (+0.0%)
ESM — tree-shaken simple (Button)69.82 kB69.81 kB✅ -13 B (-0.0%)
ESM — tree-shaken complex (ActionMenu)78.60 kB78.59 kB✅ -13 B (-0.0%)

@github-actions

github-actions Bot commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

🟢 Unit test coverage changes found

Unit test coverage has been updated through this PR.

Changes: 0 new tests, 0 removed tests, 2 improved, 0 decreased

Component/Hook Statements Functions Branches Change
ButtonGroup 88.9% 90.0% 100.0% 85.7% +1.1%
CTABanner 100.0% 100.0% 91.7% 91.8% +0.2%

@github-actions

github-actions Bot commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

🟢 No visual differences found

Our visual comparison tests did not find any differences in the UI.

@danielguillan
danielguillan marked this pull request as ready for review June 3, 2026 12:13
@danielguillan
danielguillan requested a review from a team as a code owner June 3, 2026 12:13
Copilot AI review requested due to automatic review settings June 3, 2026 12:13

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR addresses QA feedback for CTABanner styling by restoring rounded corners in non-gridline variants while keeping square edges for gridline layouts, and improves ButtonGroup so consumer-provided className is preserved alongside the component’s default styling.

Changes:

  • Apply a rounded-corners container style to CTABanner when hasGridLines is false, and add tests to validate rounded vs. gridline behavior across variants.
  • Adjust CTABanner large-viewport content spacing and add minimal-variant button group padding to better match designs.
  • Update ButtonGroup to merge className with its default class and add a unit test to prevent regressions.
Show a summary per file
File Description
packages/react/src/CTABanner/CTABanner.tsx Adds conditional rounded container class when grid lines are not enabled; ensures CTABanner.ButtonGroup forwards default + custom classes.
packages/react/src/CTABanner/CTABanner.test.tsx Adds coverage to confirm rounded corners by default across variants and explicitly not applied when hasGridLines is true.
packages/react/src/CTABanner/CTABanner.module.css Introduces CTABanner-container--rounded, adjusts large breakpoint gap behavior, and adds CTABanner-buttonGroup spacing.
packages/react/src/CTABanner/CTABanner.module.css.d.ts Updates generated CSS module typings for the new class names.
packages/react/src/ButtonGroup/ButtonGroup.tsx Merges consumer className with the default ButtonGroup class using clsx.
packages/react/src/ButtonGroup/ButtonGroup.test.tsx Adds a test ensuring custom className is forwarded alongside the default class.
.changeset/soft-banners-sparkle.md Adds a patch changeset documenting the CTABanner and ButtonGroup fixes.

Copilot's findings

  • Files reviewed: 6/52 changed files
  • Comments generated: 0

Comment thread packages/react/src/CTABanner/CTABanner.module.css Outdated

@rezrah rezrah left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks great from code perspective. Deferring to @jesussandreas for design pass. nice work @danielguillan

@danielguillan

Copy link
Copy Markdown
Collaborator Author

@copilot please resolve the branch conflicts

Copilot AI commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

@copilot please resolve the branch conflicts

Resolved by merging origin/main into this branch in 44d67676.

@jesussandreas

Copy link
Copy Markdown
Collaborator

sent you a DM but here are the notes.

reviewing the cta banner, i just have 2 comments.

  1. i cant put my finger because the specs are right, but the button spacing between text and paragraph looks off compared to design.
  • the display grid gap is adding up to the spacing, so its adding to the 16px+12px + gap making it look larger than it is.
  1. the other one is would you check the margin is this on mobile, it seems the margins are larger on mobile, they look okay on tablet.

@danielguillan
danielguillan merged commit 8433cf8 into main Jul 16, 2026
17 checks passed
@danielguillan
danielguillan deleted the danielguillan/brand-refactor-qa-cta-banner branch July 16, 2026 15:45
@primer primer Bot mentioned this pull request Jul 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants