Skip to content

[Brand Refactor] Fixes SectionIntro layout - #1360

Merged
danielguillan merged 15 commits into
mainfrom
danielguillan/brand-refactor-qa-seciton-intro
Jul 16, 2026
Merged

[Brand Refactor] Fixes SectionIntro layout#1360
danielguillan merged 15 commits into
mainfrom
danielguillan/brand-refactor-qa-seciton-intro

Conversation

@danielguillan

@danielguillan danielguillan commented Jun 2, 2026

Copy link
Copy Markdown
Collaborator

Summary

Resolves https://github.com/orgs/github/projects/23995/views/5?filterQuery=section&pane=issue&itemId=185437322
Resolves https://github.com/orgs/github/projects/23995/views/5?filterQuery=section&pane=issue&itemId=173809910
Resolves https://github.com/orgs/github/projects/23995/views/5?filterQuery=section&pane=issue&itemId=173785876

Addresses 3 different items of QA feedback from the board.

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

List of notable changes:

  • Updates SectionIntroStacked layout alignment across variants.
  • Adjusts tablet presentation for the gridline variant.
  • Refines gridline divider behavior on larger viewports.

What should reviewers focus on?

  • Verify spacing and alignment across desktop, tablet, and mobile.
  • Verify the gridline variant dividers align as expected without affecting the outer gridlines.

Steps to test:

  1. Open the SectionIntroStacked stories in the Storybook preview.
  2. Verify the default and playground stories maintain the expected content and item alignment.
  3. Verify the gridline story at mobile, tablet, and desktop widths.

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:

Please try to provide before and after screenshots or videos

Before After
image image
screenshot-008922-2026-06-02@2x image

@changeset-bot

changeset-bot Bot commented Jun 2, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: ad8dccc

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 2, 2026

Copy link
Copy Markdown
Contributor

🟢 No design token changes found

@github-actions

github-actions Bot commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

🟢 Bundle size report

CheckMainBranchChange
UMD — full bundle (JS)101.36 kB101.34 kB✅ -22 B (-0.0%)
UMD — full bundle (CSS)66.82 kB66.91 kB⬆️ +91 B (+0.1%)
ESM — full bundle (JS + CSS)1.51 MB1.51 MB⬆️ +122 B (+0.0%)
ESM — tree-shaken simple (Button)69.82 kB69.82 kB🟢 No change
ESM — tree-shaken complex (ActionMenu)78.60 kB78.60 kB🟢 No change

@github-actions

github-actions Bot commented Jun 2, 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, 1 improved, 1 decreased

Component/Hook Statements Functions Branches Change
IDE 91.2% 91.6% 97.4% 82.8% 83.4% +0.7%
SectionIntroStacked 98.4% 100.0% 95.8% 95.5% -0.4%

@github-actions

github-actions Bot commented Jun 2, 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 2, 2026 17:24
@danielguillan
danielguillan requested a review from a team as a code owner June 2, 2026 17:24
Copilot AI review requested due to automatic review settings June 2, 2026 17:24

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 refines SectionIntroStacked layout behavior to address QA feedback around alignment and gridline divider presentation across responsive breakpoints.

Changes:

  • Standardizes the SectionIntroStacked grid column layout and removes inter-column gap via columnGap="none".
  • Updates SectionIntroStacked responsive spacing rules, including new tablet-specific layout constraints and gridline adjustments.
  • Tweaks spacing in the FlexSuite AI overview recipe to better match the updated section intro layout.
Show a summary per file
File Description
packages/react/src/SectionIntroStacked/SectionIntroStacked.tsx Simplifies/stabilizes the grid column layout and sets columnGap="none" for alignment consistency.
packages/react/src/SectionIntroStacked/SectionIntroStacked.module.css Introduces new responsive spacing rules and adjusts gridline divider behavior across breakpoints.
packages/react/src/recipes/Flexsuite/Overview/FlexSuiteAIOverview.tsx Adjusts section padding to maintain consistent vertical rhythm after layout updates.
.changeset/soft-clouds-shift.md Publishes the spacing/layout adjustment as a patch changeset for @primer/react-brand.

Copilot's findings

  • Files reviewed: 4/21 changed files
  • Comments generated: 2

Comment on lines +13 to +14
max-width: 618px;
margin-inline: auto;

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@rezrah I ended up using a max-width instead of the inline paddings, following @jesussandreas's comment on the QA ticket.

@jesussandreas, can you please confirm that this is the correct max-width for tablet viewports?

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.

@danielguillan literally just seeing this. but it seems correct.

@danielguillan

Copy link
Copy Markdown
Collaborator Author

@copilot resolve the merge conflicts in this pull request

Copilot AI commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

@copilot resolve the merge conflicts in this pull request

Done — I merged origin/main into this branch and resolved the merge conflicts in commit b7b19691.

@jesussandreas

Copy link
Copy Markdown
Collaborator

another change here @danielguillan

can you ensure the gap is set to 64px in mobile? please.

Screenshot 2026-06-16 at 10 39 11 AM

this gap should be 12px instead of 8px.
Screenshot 2026-06-16 at 10 40 16 AM

border radius should be 12px.
Screenshot 2026-06-16 at 10 40 55 AM

total inner padding, should be 20px, currently is set to 32px~
Screenshot 2026-06-16 at 10 42 02 AM


on tablet, the gap is 40px instead of 64px.
Screenshot 2026-06-16 at 10 44 02 AM

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.

@danielguillan @jesussandreas the space under the section intro is now really large on older templates like flex suite.. checkout this snapshot diff out for an example 👇.

Personally feel the spacing is too much even on gridline, but it looks dramatically worse on older termplates. Could/should we constrain or reduce it?


@media screen and (min-width: 48rem) and (max-width: 63.24rem) {
.SectionIntroStacked {
max-width: 618px;

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.

Just curious whether we're accounting for some kind of inline borders here? Is that why it's not rounded up to 620px?

}

@media screen and (max-width: 63.25rem) {
@media screen and (max-width: 63.24rem) {

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.

@danielguillan what was the reason for this breakpoint change? Want to understand the reason so we can consider fixing the inconsistencies in other components. We same to have more components using max-width of 63.25rem instead of 63.24rem right now.

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.

@danielguillan is this intended for this PR?

@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.

Code looks good. Some minor feedback, but ✅ to unblock.

@jesussandreas can you please 👍 on your side too if you're ready for this to land.

@danielguillan

Copy link
Copy Markdown
Collaborator Author

@copilot resolve the merge conflicts in this pull request

Copilot AI commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

@copilot resolve the merge conflicts in this pull request

Done — I merged origin/main into this branch and resolved the merge conflicts in commit 7c29d112.

@danielguillan
danielguillan merged commit 5cbb414 into main Jul 16, 2026
17 checks passed
@danielguillan
danielguillan deleted the danielguillan/brand-refactor-qa-seciton-intro branch July 16, 2026 14:09
@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