Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/pink-windows-add.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@primer/react": minor
---

Adds `hasBorder` prop to PageHeader to allow a bottom border
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
18 changes: 18 additions & 0 deletions e2e/components/PageHeader.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,24 @@ test.describe('PageHeader', () => {
}
})

test.describe('Has Border', () => {
for (const theme of themes) {
test.describe(theme, () => {
test('default @vrt', async ({page}) => {
await visit(page, {
id: 'components-pageheader-features--has-bottom-border',
globals: {
colorScheme: theme,
},
})

// Default state
expect(await page.screenshot()).toMatchSnapshot(`PageHeader.Has Border.${theme}.png`)
})
})
}
})

test.describe('Has Large Title', () => {
for (const theme of themes) {
test.describe(theme, () => {
Expand Down
5 changes: 5 additions & 0 deletions packages/react/src/PageHeader/PageHeader.docs.json
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,11 @@
"name": "as",
"type": "React.ElementType",
"defaultValue": "\"div\""
},
{
"name": "hasBorder",
"type": "boolean",
"description": "Whether to render a border below the PageHeader. This border will NOT be rendered if the PageHeader has a `PageHeader.Navigation` child that is not hidden at the current breakpoint."
}
],
"subcomponents": [
Expand Down
10 changes: 10 additions & 0 deletions packages/react/src/PageHeader/PageHeader.features.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -266,4 +266,14 @@ export const WithActionsThatHaveResponsiveContent = () => (
</Box>
)

export const HasBottomBorder = () => (
<Box sx={{padding: 3}}>
<PageHeader role="banner" aria-label="Title" hasBorder>
<PageHeader.TitleArea>
<PageHeader.Title>Title</PageHeader.Title>
</PageHeader.TitleArea>
</PageHeader>
</Box>
)

export default meta
27 changes: 27 additions & 0 deletions packages/react/src/PageHeader/PageHeader.module.css
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,33 @@
--title-line-height: var(--custom-line-height, var(--text-title-lineHeight-medium, 1.6));
}

&[data-has-border='true']:has([data-component='PH_Navigation'][data-hidden-all]),
&[data-has-border='true']:not(:has([data-component='PH_Navigation'])) {
border-block-end: var(--borderWidth-thin) solid var(--borderColor-default);
padding-block-end: var(--base-size-8);
}

@media screen and (max-width: 768px) {
&[data-has-border='true']:has([data-component='PH_Navigation'][data-hidden-narrow]) {
border-block-end: var(--borderWidth-thin) solid var(--borderColor-default);
padding-block-end: var(--base-size-8);
}
}

@media screen and (min-width: 768px) {
&[data-has-border='true']:has([data-component='PH_Navigation'][data-hidden-regular]) {
border-block-end: var(--borderWidth-thin) solid var(--borderColor-default);
padding-block-end: var(--base-size-8);
}
}

@media screen and (min-width: 1440px) {
&[data-has-border='true']:has([data-component='PH_Navigation'][data-hidden-wide]) {
border-block-end: var(--borderWidth-thin) solid var(--borderColor-default);
padding-block-end: var(--base-size-8);
}
}

& [data-component='PH_LeadingAction'],
& [data-component='PH_TrailingAction'],
& [data-component='PH_Actions'],
Expand Down
5 changes: 4 additions & 1 deletion packages/react/src/PageHeader/PageHeader.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,9 @@ const meta: Meta<typeof PageHeader> = {
description:
'ContextArea is only visible on narrow viewports by default to provide user context of where they are at their journey.',
},
hasBorder: {
type: 'boolean',
},
ParentLink: {
type: 'string',
if: {arg: 'hasContextArea'},
Expand Down Expand Up @@ -189,7 +192,7 @@ export default meta

export const Playground: StoryFn = args => (
<Box sx={{padding: 3}}>
<PageHeader aria-label={args.Title} role="banner">
<PageHeader aria-label={args.Title} role="banner" hasBorder={args.hasBorder}>
<PageHeader.TitleArea
variant={{
narrow: args['Title.variant'],
Expand Down
32 changes: 30 additions & 2 deletions packages/react/src/PageHeader/PageHeader.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -75,10 +75,11 @@ export type PageHeaderProps = {
as?: React.ElementType | 'header' | 'div'
className?: string
role?: AriaRole
hasBorder?: boolean
} & SxProp

const Root = React.forwardRef<HTMLDivElement, React.PropsWithChildren<PageHeaderProps>>(
({children, className, sx = {}, as = 'div', 'aria-label': ariaLabel, role}, forwardedRef) => {
({children, className, sx = {}, as = 'div', 'aria-label': ariaLabel, role, hasBorder}, forwardedRef) => {
const enabled = useFeatureFlag(CSS_MODULES_FEATURE_FLAG)

const rootStyles = {
Expand Down Expand Up @@ -116,6 +117,29 @@ const Root = React.forwardRef<HTMLDivElement, React.PropsWithChildren<PageHeader
{
height: 'calc(var(--title-line-height) * 1em)',
},
'&[data-has-border="true"]:has([data-component="PH_Navigation"][data-hidden-all]), &[data-has-border="true"]:not(:has([data-component="PH_Navigation"]))':
{
borderBlockEnd: 'var(--borderWidth-thin) solid var(--borderColor-default)',
paddingBlockEnd: 'var(--base-size-8)',
},
'@media screen and (max-width: 768px)': {
'&[data-has-border="true"]:has([data-component="PH_Navigation"][data-hidden-narrow])': {
borderBlockEnd: 'var(--borderWidth-thin) solid var(--borderColor-default)',
paddingBlockEnd: 'var(--base-size-8)',
},
},
'@media screen and (min-width: 768px)': {
'&[data-has-border="true"]:has([data-component="PH_Navigation"][data-hidden-regular])': {
borderBlockEnd: 'var(--borderWidth-thin) solid var(--borderColor-default)',
paddingBlockEnd: 'var(--base-size-8)',
},
},
'@media screen and (min-width: 1440px)': {
'&[data-has-border="true"]:has([data-component="PH_Navigation"][data-hidden-wide])': {
borderBlockEnd: 'var(--borderWidth-thin) solid var(--borderColor-default)',
paddingBlockEnd: 'var(--base-size-8)',
},
},
}

const rootRef = useProvidedRefOrCreate<HTMLDivElement>(forwardedRef as React.RefObject<HTMLDivElement>)
Expand Down Expand Up @@ -176,6 +200,7 @@ const Root = React.forwardRef<HTMLDivElement, React.PropsWithChildren<PageHeader
ref={rootRef}
as={as}
className={clsx(enabled && classes.PageHeader, className)}
data-has-border={hasBorder ? 'true' : undefined}
sx={enabled ? sx : merge<BetterSystemStyleObject>(rootStyles, sx)}
aria-label={ariaLabel}
role={role}
Expand Down Expand Up @@ -754,6 +779,7 @@ const Navigation: React.FC<React.PropsWithChildren<NavigationProps>> = ({
aria-label={as === 'nav' ? ariaLabel : undefined}
aria-labelledby={as === 'nav' ? ariaLabelledBy : undefined}
className={clsx(enabled && classes.Navigation, className)}
data-component="PH_Navigation"
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
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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

@TylerJDev TylerJDev Mar 19, 2025

Choose a reason for hiding this comment

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

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.

sx={
enabled
? sx
Expand All @@ -773,7 +799,9 @@ const Navigation: React.FC<React.PropsWithChildren<NavigationProps>> = ({
sx,
)
}
{...getHiddenDataAttributes(enabled, hidden)}
// passing `true` always get the data attributes for the hidden prop,
// not just for CSS modules
{...getHiddenDataAttributes(true, hidden)}
>
{children}
</Box>
Expand Down
Loading