Add prop to PageHeader to allow a bottom border by mperrotti · Pull Request #5741 · primer/react

mperrotti · 2025-03-06T21:15:58Z

Relates to https://github.com/github/primer/issues/2837

Changelog

New

hasBorder prop on PageHeader

Changed

Removed

Rollout strategy

Patch release
Minor release
Major release; if selected, include a written rollout or migration plan
None; if selected, include a brief description as to why

Testing & Reviewing

Merge checklist

Added/updated tests
Added/updated documentation
Added/updated previews (Storybook)
Changes are SSR compatible
Tested in Chrome
Tested in Firefox
Tested in Safari
Tested in Edge
(GitHub staff only) Integration tests pass at github/github (Learn more about how to run integration tests)

changeset-bot · 2025-03-06T21:16:01Z

🦋 Changeset detected

Latest commit: 18f09dd

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

This PR includes changesets to release 1 package

Name	Type
@primer/react	Minor

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 · 2025-03-06T21:16:20Z

Uh oh! @mperrotti, at least one image you shared is missing helpful alt text. Check your pull request body to fix the following violations:

Images should have meaningful alternative text (alt text) at line 7

Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image.

Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.

🤖 Beep boop! This comment was added automatically by github/accessibility-alt-text-bot.

github-actions · 2025-03-06T21:16:28Z

👋 Hi, this pull request contains changes to the source code that github/github depends on. If you are GitHub staff, we recommend testing these changes with github/github using the integration workflow. Thanks!

github-actions · 2025-03-06T21:19:36Z

size-limit report 📦

Path	Size
packages/react/dist/browser.esm.js	105.57 KB (+0.09% 🔺)
packages/react/dist/browser.umd.js	105.94 KB (+0.09% 🔺)

joshblack · 2025-03-07T02:18:26Z

 aria-label={as === 'nav' ? ariaLabel : undefined}
 aria-labelledby={as === 'nav' ? ariaLabelledBy : undefined}
 className={clsx(enabled && classes.Navigation, className)}
+ data-component="PH_Navigation"


Suggested change
data-component="PH_Navigation"
data-component="PageHeader.Navigation"
One suggestion that could make it more clear what the component is when coming back to this later

PH_{Subcomponent} is the naming pattern we use for the other subcomponents. Do you still think we should change it?

I think we're kind of trying to move away from data-component actually.. what about just giving it a classname instead?

I agree, PageHeader.Navigation is more clear, but to keep it consistent I think PH_{Subcomponent} is fine too! Would be curious about the className route too 👀 Maybe we'll want to revisit in a different PR to adjust the existing data-* attributes.

lukasoppermann · 2025-03-07T09:21:18Z

@mperrotti somehow I can't add this comment to the code, but we need this: hasBorder={args.hasBorder} on the Playground story: https://github.com/primer/react/pull/5741/files#diff-38be7109ab257cb99d6f1950aa251d530e55a99c8705aaf94ff68f2d98c82897R195

lukasoppermann · 2025-03-07T09:23:18Z

Also it is buggy for those two cases:

I wonder if we should disable those cases, so that lines can not be added when a Navigation or description is enabled?

But for a normal text description it could be useful.

Co-authored-by: Josh Black <joshblack@github.com>

mperrotti · 2025-03-10T12:51:09Z

@lukasoppermann - I thought I handled those cases already, but I must have broke something. I'll take another look.

…o mp/pageheader-hasBorder

…Border

emilybrick

Snapshots look good here & in storybook

I'm not sure if the naming pattern discussion is resolved, but story + snapshots lgtm 👍

TylerJDev

LGTM!

mperrotti added 2 commits March 6, 2025 16:11

adds prop to PageHeader to add a bottom border
de8e90d

adds changeset
47c50c7

mperrotti requested a review from a team as a code owner March 6, 2025 21:15

mperrotti requested a review from joshblack March 6, 2025 21:15

github-actionsBot added the integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm label Mar 6, 2025

github-actionsBot requested a deployment to storybook-preview-5741 March 6, 2025 21:20 Abandoned

mperrotti deployed to github-pages March 6, 2025 21:33 — with GitHub Actions View deployment

github-actionsBottemporarily deployed to storybook-preview-5741 March 6, 2025 21:33 Inactive

joshblack reviewed Mar 7, 2025
View reviewed changes

primer deleted a comment from github-actionsBotMar 7, 2025

Update packages/react/src/PageHeader/PageHeader.tsx
7029c0f
Co-authored-by: Josh Black <joshblack@github.com>

mperrotti deployed to github-pages March 10, 2025 13:03 — with GitHub Actions View deployment

github-actionsBottemporarily deployed to storybook-preview-5741 March 10, 2025 13:03 Inactive

mperrotti added 3 commits March 10, 2025 10:34

got smarter about when we hide and show the bottom border
13f0797

Merge branch 'mp/pageheader-hasBorder' of github.com:primer/react int…
9b8fe10
…o mp/pageheader-hasBorder

Merge branch 'main' of github.com:primer/react into mp/pageheader-has…
3713e67
…Border

mperrotti deployed to github-pages March 10, 2025 14:48 — with GitHub Actions View deployment

github-actionsBottemporarily deployed to storybook-preview-5741 March 10, 2025 14:49 Inactive

lukasoppermann mentioned this pull request Mar 11, 2025
Add Figma Code connect PageHeader #5757
Merged
13 tasks

Merge branch 'main' into mp/pageheader-hasBorder
5c11ae1

mperrotti deployed to github-pages March 11, 2025 21:02 — with GitHub Actions View deployment

github-actionsBottemporarily deployed to storybook-preview-5741 March 11, 2025 21:02 Inactive

Merge branch 'main' into mp/pageheader-hasBorder
6705b40

primerBot requested a review from emilybrick March 18, 2025 21:03

github-actionsBot removed the update snapshots 🤖 Command that updates VRT snapshots on the pull request label Mar 18, 2025

github-actionsBottemporarily deployed to storybook-preview-5741 March 18, 2025 21:09 Inactive

primerBotdeployed to github-pages March 18, 2025 21:23 View deployment

github-actionsBottemporarily deployed to storybook-preview-5741 March 18, 2025 21:23 Inactive

mperrotti added the update snapshots 🤖 Command that updates VRT snapshots on the pull request label Mar 18, 2025

github-actionsBot removed the update snapshots 🤖 Command that updates VRT snapshots on the pull request label Mar 18, 2025

primerBotdeployed to github-pages March 19, 2025 14:53 View deployment

github-actionsBottemporarily deployed to storybook-preview-5741 March 19, 2025 14:53 Inactive

emilybrick approved these changes Mar 19, 2025
View reviewed changes

mperrotti enabled auto-merge (squash) March 19, 2025 17:10

TylerJDev approved these changes Mar 19, 2025
View reviewed changes

Merge branch 'main' into mp/pageheader-hasBorder
52eecc8

mperrotti deployed to github-pages March 21, 2025 13:51 — with GitHub Actions View deployment

github-actionsBottemporarily deployed to storybook-preview-5741 March 21, 2025 13:52 Inactive

mperrotti added the update snapshots 🤖 Command that updates VRT snapshots on the pull request label Mar 21, 2025

github-actionsBot removed the update snapshots 🤖 Command that updates VRT snapshots on the pull request label Mar 21, 2025

mperrotti deployed to github-pages March 25, 2025 16:06 — with GitHub Actions View deployment

github-actionsBottemporarily deployed to storybook-preview-5741 March 25, 2025 16:06 Inactive

Merge branch 'main' into mp/pageheader-hasBorder
18f09dd

mperrotti deployed to github-pages March 25, 2025 17:17 — with GitHub Actions View deployment

github-actionsBottemporarily deployed to storybook-preview-5741 March 25, 2025 17:17 Inactive

TylerJDev disabled auto-merge March 25, 2025 17:31

TylerJDev added this pull request to the merge queue Mar 25, 2025

TylerJDev removed this pull request from the merge queue due to a manual request Mar 25, 2025

mperrotti added this pull request to the merge queue Mar 25, 2025

Merged via the queue into main with commit d32dd27Mar 25, 2025

mperrotti deleted the mp/pageheader-hasBorder branch March 25, 2025 17:44

primerBot mentioned this pull request Mar 25, 2025
Release tracking (37.17.0) #5802
Merged

	data-component="PH_Navigation"
	data-component="PageHeader.Navigation"

Uh oh!

Conversation

mperrotti commented Mar 6, 2025

Changelog

New

Changed

Removed

Rollout strategy

Testing & Reviewing

Merge checklist

Uh oh!

changeset-botBot commented Mar 6, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

github-actionsBot commented Mar 6, 2025

Uh oh!

github-actionsBot commented Mar 6, 2025

Uh oh!

github-actionsBot commented Mar 6, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

size-limit report 📦

Uh oh!

Uh oh!

joshblackMar 7, 2025

Choose a reason for hiding this comment

Uh oh!

mperrottiMar 10, 2025

Choose a reason for hiding this comment

Uh oh!

langermankMar 18, 2025

Choose a reason for hiding this comment

Uh oh!

TylerJDevMar 19, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lukasoppermann commented Mar 7, 2025

Uh oh!

lukasoppermann commented Mar 7, 2025

Uh oh!

mperrotti commented Mar 10, 2025

Uh oh!

emilybrick left a comment

Choose a reason for hiding this comment

Uh oh!

TylerJDev left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

changeset-botBot commented Mar 6, 2025•
edited
Loading

github-actionsBot commented Mar 6, 2025•
edited
Loading

TylerJDevMar 19, 2025•
edited
Loading