Skip to content

Add align prop to Dialog component#7594

Merged
francinelucca merged 15 commits intomainfrom
copilot/add-vertical-positioning-prop
Mar 4, 2026
Merged

Add align prop to Dialog component#7594
francinelucca merged 15 commits intomainfrom
copilot/add-vertical-positioning-prop

Conversation

Copy link
Contributor

Copilot AI commented Feb 25, 2026

Closes https://github.com/github/primer/issues/6439

Dialog only controlled horizontal positioning. Developers needing vertical control resorted to CSS hacks using :has() pseudo-selectors on the backdrop. This adds a first-class align prop that works when position is "center" (the default).

Changelog

New

  • align prop on Dialog: 'top' | 'center' | 'bottom' — controls vertical alignment of the dialog within the viewport when position is "center" (the default). The default offset margin (var(--base-size-64)) is applied on the dialog element itself and can be overridden via a consumer-supplied className.

Changed

  • Dialog.module.cssdata-align margin rules are scoped inside the [data-position-regular='center'] block so they only apply when position is "center". Non-center positions (left, right) take precedence and align has no visual effect. At narrow breakpoints, align overrides are allowed only when data-position-narrow='center'; bottom and fullscreen narrow positions have no align effect — both the backdrop align-items and dialog margins are unaffected. CSS specificity is correctly ordered throughout: backdrop align rules use :where() to zero out :not() specificity so narrow-position rules always win by source order; dialog margin reset rules for bottom/fullscreen compile to higher specificity than the margin-setting rule.

Removed

  • AlignTopWithHorizontalPosition story — removed because align does not apply when position is non-center.

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

Two Storybook stories cover top and bottom alignment (with default center position). Unit tests assert correct data-align attribute on both dialog and backdrop elements. VRT covers top and bottom variants across themes.

Usage:

// Snap to top (position defaults to center)
<Dialog align="top" ... />

// Near bottom
<Dialog align="bottom" ... />

// Override default margin
<Dialog align="top" className={styles.customMargin} ... />

Omitting align leaves existing behavior unchanged (centered). Specifying a non-center position ("left", "right") or a narrow position of "bottom" or "fullscreen" causes align to have no visual effect on either the backdrop or the dialog element.

Merge checklist

Original prompt

This section details on the original issue you should resolve

<issue_title>[Proposal] Add vertical positioning prop to Dialog component</issue_title>
<issue_description>### Describe the pattern or pattern updates you are proposing

Add a new prop to the Dialog component that allows modification of vertical positioning. Proposed values for the prop are:

  • 'top': positions the Dialog ~4rem from the top of the screen, horizontally centered
  • 'bottom': positions the Dialog near the bottom of the screen, horizontally centered
  • 'center': (default) vertically centers the Dialog on the screen

This prop will allow developers to position dialogs without relying on CSS hacks or pseudo-selectors.

Where will the pattern be used, or is already used?

Dialogs in applications where vertical position needs to be customized based on context (e.g., notifications, onboarding, custom UX requirements).

Why is this pattern needed?

Currently, Dialog has a position prop, but it only controls horizontal positioning. There is no built-in way to control vertical position, which leads to workarounds using CSS pseudo-selectors that are hacky and may not be robust. Adding this prop will improve developer experience and flexibility.

Are there sketches, wireframes, mockups, or prototypes?

None available yet; usage would be similar to existing positioning props.

Is this pattern needed for a launch event or other deadline? Is this pattern critical to the success of a feature ship?

No specific deadline, but adding this soon would help avoid hacks in user code.

Do you have design and/or engineering resources available to work on this? Or are you requesting the Primer team to take on the full scope of work?

Requesting Primer team to assess, design, and implement this modification.

What other ways (if any) might you be able to achieve your goals if we can't complete this work in your timeframe?

As a workaround, vertical positioning can be achieved with margin and CSS pseudo-selectors, but it's not ideal and can be brittle.

Additional context

Relevant Slack discussion: Dialog vertical position enhancement request.

User hack example:

.dialog {
  margin-top: 4rem;

  *:has(&) {
    align-items: flex-start;
  }
}

This approach works but is not robust or recommended for production.

View original Slack conversation</issue_description>

<agent_instructions>implement proposal, add stories and tests to demo and test the functionality. Ensure that the new solution works seamlessly with already existing horizontal positioning, and that you can use both together or none and the component still behaves as expected. Also add any new props to the dialog.docs.json file and add at least one new story to e2e testing </agent_instructions>

Comments on the Issue (you are @copilot in this section)

  • Fixes github/primer#6439

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

@changeset-bot
Copy link

changeset-bot bot commented Feb 25, 2026

🦋 Changeset detected

Latest commit: 5ee2809

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

Co-authored-by: francinelucca <40550942+francinelucca@users.noreply.github.com>
Copilot AI changed the title [WIP] Add vertical positioning prop to Dialog component Add verticalPosition prop to Dialog component Feb 25, 2026
@github-actions github-actions bot added the integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm label Feb 25, 2026
@github-actions
Copy link
Contributor

⚠️ Action required

👋 Hi, this pull request contains changes to the source code that github/github-ui depends on. If you are GitHub staff, test these changes with github/github-ui using the integration workflow. Or, apply the integration-tests: skipped manually label to skip these checks.

@francinelucca
Copy link
Member

@copilot ensure consumers are able to override the amount of padding through a supplied Dialog classname, which means you should put the margin inside the dialog, not the backdrop. Also data-position-narrow should not override the align-items prop if a verticalPosition is set

… in narrow breakpoint

Co-authored-by: francinelucca <40550942+francinelucca@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Feb 25, 2026

@copilot ensure consumers are able to override the amount of padding through a supplied Dialog classname, which means you should put the margin inside the dialog, not the backdrop. Also data-position-...

Done in b2aa8eb. Two changes made to Dialog.module.css:

  1. Padding moved to dialog: padding-top/padding-bottom removed from the backdrop; replaced with margin-top/margin-bottom on the .Dialog element, so consumers can override via a className supplied to <Dialog>.

  2. Narrow breakpoint protected: data-position-narrow='center' and data-position-narrow='bottom' rules in the narrow media query now use :not([data-vertical-position]) for the align-items property, so verticalPosition always wins. The justify-content from narrow positioning is preserved via a companion rule that matches when both attributes are present.

@francinelucca
Copy link
Member

@copilot rename new prop to "align"

francinelucca and others added 2 commits February 25, 2026 17:13
Co-authored-by: francinelucca <40550942+francinelucca@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Feb 25, 2026

@copilot rename new prop to "align"

Done in 8041645. Renamed verticalPositionalign and data-vertical-positiondata-align across Dialog.tsx, Dialog.module.css, Dialog.features.stories.tsx, Dialog.test.tsx, Dialog.docs.json, and e2e/components/Dialog.test.ts. Story exports are now AlignTop, AlignBottom, and AlignTopWithHorizontalPosition.

@github-actions github-actions bot requested a deployment to storybook-preview-7594 February 25, 2026 22:17 Abandoned
Copilot AI changed the title Add verticalPosition prop to Dialog component Add align prop to Dialog component Feb 25, 2026
@github-actions github-actions bot temporarily deployed to storybook-preview-7594 February 25, 2026 22:26 Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-7594 February 26, 2026 01:56 Inactive
@francinelucca francinelucca added the update snapshots 🤖 Command that updates VRT snapshots on the pull request label Feb 26, 2026
@francinelucca francinelucca marked this pull request as ready for review February 26, 2026 02:34
@github-actions github-actions bot requested a deployment to storybook-preview-7594 February 27, 2026 13:49 Abandoned
@github-actions github-actions bot temporarily deployed to storybook-preview-7594 February 27, 2026 14:03 Inactive
@francinelucca francinelucca added this pull request to the merge queue Mar 3, 2026
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 3, 2026
@francinelucca francinelucca added this pull request to the merge queue Mar 3, 2026
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 3, 2026
@francinelucca francinelucca added the update snapshots 🤖 Command that updates VRT snapshots on the pull request label Mar 3, 2026
@github-actions github-actions bot temporarily deployed to storybook-preview-7594 March 3, 2026 15:33 Inactive
@github-actions github-actions bot removed the update snapshots 🤖 Command that updates VRT snapshots on the pull request label Mar 3, 2026
@github-actions github-actions bot temporarily deployed to storybook-preview-7594 March 3, 2026 15:54 Inactive
@primer-integration
Copy link

👋 Hi from github/github-ui! Your integration PR is ready: https://github.com/github/github-ui/pull/15020

@primer-integration
Copy link

Integration test results from github/github-ui:

Passed  CI   Passed
Passed  VRT   Passed
Passed  Projects   Passed

All checks passed!

@francinelucca francinelucca added this pull request to the merge queue Mar 4, 2026
Merged via the queue into main with commit 1e54bdf Mar 4, 2026
52 checks passed
@francinelucca francinelucca deleted the copilot/add-vertical-positioning-prop branch March 4, 2026 15:03
@primer primer bot mentioned this pull request Mar 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants