Skip to content
Open
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/neat-moose-dress.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@primer/react': patch
---

Dialog: dynamically switch footer button layout based on available height.
12 changes: 6 additions & 6 deletions packages/react/src/Dialog/Dialog.module.css
Original file line number Diff line number Diff line change
Expand Up @@ -389,11 +389,11 @@ Add a border between the body and footer if:
padding: var(--base-size-16);
gap: var(--base-size-8);
flex-shrink: 0;
}

@media (max-height: 325px) {
flex-wrap: nowrap;
overflow-x: scroll;
flex-direction: row;
justify-content: unset;
}
.Dialog[data-footer-button-layout='scroll'] .Footer {
flex-wrap: nowrap;
overflow-x: scroll;
flex-direction: row;
justify-content: unset;
}
79 changes: 78 additions & 1 deletion packages/react/src/Dialog/Dialog.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import type {ForwardRefComponent as PolymorphicForwardRefComponent} from '../uti
import classes from './Dialog.module.css'
import {clsx} from 'clsx'
import {useSlots} from '../hooks/useSlots'
import {useResizeObserver} from '../hooks/useResizeObserver'

/* Dialog Version 2 */

Expand Down Expand Up @@ -239,6 +240,41 @@ const defaultPosition = {
}

const defaultFooterButtons: Array<DialogButtonProps> = []
// Minimum room needed for body content before forcing footer buttons into horizontal scroll.
const MIN_BODY_HEIGHT = 56

// Measures what the footer height would be in wrap mode by cloning it offscreen,
// so we can decide layout without mutating the visible footer.
function measureWrappedFooterHeight(footerElement: HTMLElement) {
const measurementContainer = document.createElement('div')
const measuredFooter = footerElement.cloneNode(true) as HTMLElement

Object.assign(measurementContainer.style, {
position: 'fixed',
top: '0',
left: '-99999px',
visibility: 'hidden',
pointerEvents: 'none',
contain: 'layout style size',
})

measuredFooter.style.width = `${footerElement.getBoundingClientRect().width}px`

Object.assign(measuredFooter.style, {
flexWrap: 'wrap',
overflowX: '',
overflowY: '',
justifyContent: '',
})

measurementContainer.appendChild(measuredFooter)
document.body.appendChild(measurementContainer)

const measuredHeight = measuredFooter.offsetHeight
measurementContainer.remove()

Comment on lines +248 to +275
Copy link

Copilot AI Mar 19, 2026

Choose a reason for hiding this comment

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

measureWrappedFooterHeight clones the entire footer, inserts it into document.body, forces layout, then removes it on every resize observer callback. This can be relatively expensive during viewport changes (especially on mobile where viewport height can change frequently). Consider caching/reusing a single measurement container and only re-measuring when width/footer content changes, to reduce DOM churn and forced reflow.

Copilot uses AI. Check for mistakes.
return measuredHeight
}

// useful to determine whether we're inside a Dialog from a nested component
export const DialogContext = React.createContext<object | undefined>(undefined)
Expand Down Expand Up @@ -273,6 +309,7 @@ const _Dialog = React.forwardRef<HTMLDivElement, React.PropsWithChildren<DialogP
}
}
const [lastMouseDownIsBackdrop, setLastMouseDownIsBackdrop] = useState<boolean>(false)
const [footerButtonLayout, setFooterButtonLayout] = useState<'scroll' | 'wrap'>('wrap')
const defaultedProps = {...props, title, subtitle, role, dialogLabelId, dialogDescriptionId}
const onBackdropClick = useCallback(
(e: SyntheticEvent) => {
Expand Down Expand Up @@ -339,6 +376,45 @@ const _Dialog = React.forwardRef<HTMLDivElement, React.PropsWithChildren<DialogP
const header = slots.header ?? (renderHeader ?? DefaultHeader)(defaultedProps)
const body = slots.body ?? (renderBody ?? DefaultBody)({...defaultedProps, children: childrenWithoutSlots})
const footer = slots.footer ?? (renderFooter ?? DefaultFooter)(defaultedProps)
const hasFooter = footer != null

const updateFooterButtonLayout = useCallback(() => {
if (!hasFooter) {
setFooterButtonLayout('wrap')
return
}

const dialogElement = dialogRef.current
if (!(dialogElement instanceof HTMLElement)) {
return
}

const headerElement = dialogElement.querySelector(`.${classes.Header}`)
const footerElement = dialogElement.querySelector(`.${classes.Footer}`)

if (!(footerElement instanceof HTMLElement)) {
return
}

const viewportHeight = backdropRef.current?.clientHeight ?? window.innerHeight
const positionRegular = dialogElement.getAttribute('data-position-regular')
const positionNarrow = dialogElement.getAttribute('data-position-narrow')
// fullscreen/left/right fill the full viewport; otherwise match CSS max-height gutter.
const gutter = viewportHeight <= 280 ? 12 : 64
const dialogMaxHeight =
positionNarrow === 'fullscreen' || positionRegular === 'left' || positionRegular === 'right'
? viewportHeight
: Math.max(0, viewportHeight - gutter)

Comment on lines +399 to +408
Copy link

Copilot AI Mar 19, 2026

Choose a reason for hiding this comment

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

The gutter/dialogMaxHeight calculation re-implements CSS max-height logic, but it doesn’t match the stylesheet: 12px gutter is only applied under @media (max-width: 767px) { @media (max-height: 280px) { ... } } in Dialog.module.css, while this code applies 12 whenever viewportHeight <= 280 regardless of viewport width. This can cause the footer layout decision to diverge from the actual dialog max-height. Consider deriving the max height from computed styles (e.g. getComputedStyle(dialogElement).maxHeight) or mirroring the same width breakpoint check before switching to the 12px gutter.

Copilot uses AI. Check for mistakes.
const headerHeight = headerElement instanceof HTMLElement ? headerElement.offsetHeight : 0
const wrappedFooterHeight = measureWrappedFooterHeight(footerElement)
const visibleBodyHeightWithWrap = Math.max(0, dialogMaxHeight - headerHeight - wrappedFooterHeight)

setFooterButtonLayout(visibleBodyHeightWithWrap >= MIN_BODY_HEIGHT ? 'wrap' : 'scroll')
}, [hasFooter])

Copy link

Copilot AI Mar 19, 2026

Choose a reason for hiding this comment

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

updateFooterButtonLayout is only triggered via useResizeObserver(..., backdropRef), so the layout may become stale when footer/header content changes without a viewport/backdrop resize (e.g. button label changes, async loading states, toggling footerButtons). Additionally, in the useResizeObserver fallback path (no ResizeObserver), the callback isn’t invoked on mount—only on window.resize—so data-footer-button-layout may never be set correctly initially. Consider calling updateFooterButtonLayout() in an effect when relevant inputs change (and/or observing the dialog/footer element too) so the layout decision is updated even without a resize event.

Suggested change
useEffect(() => {
updateFooterButtonLayout()
}, [updateFooterButtonLayout])

Copilot uses AI. Check for mistakes.
useResizeObserver(updateFooterButtonLayout, backdropRef)

const positionDataAttributes =
typeof position === 'string'
? {'data-position-regular': position}
Expand Down Expand Up @@ -371,7 +447,8 @@ const _Dialog = React.forwardRef<HTMLDivElement, React.PropsWithChildren<DialogP
{...(align && {'data-align': align})}
data-width={width}
data-height={height}
data-has-footer={footer != null ? '' : undefined}
data-has-footer={hasFooter ? '' : undefined}
data-footer-button-layout={hasFooter ? footerButtonLayout : undefined}
Comment on lines +450 to +451
Copy link

Copilot AI Mar 19, 2026

Choose a reason for hiding this comment

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

This PR introduces a new data-footer-button-layout attribute that changes footer behavior based on measured heights, but Dialog.test.tsx doesn’t currently cover this new behavior. Adding a unit test that mocks ResizeObserver and element measurements (e.g. offsetHeight / getBoundingClientRect) would help prevent regressions for the wrap-vs-scroll switching logic.

Copilot uses AI. Check for mistakes.
className={clsx(className, classes.Dialog)}
style={style}
>
Expand Down
Loading