Skip to content

fix(components): remove extra popover padding for DialogTrigger with menu/listbox#1884

Draft
cpurps22 wants to merge 3 commits intomainfrom
devin/1776277270-fix-menu-popover-padding
Draft

fix(components): remove extra popover padding for DialogTrigger with menu/listbox#1884
cpurps22 wants to merge 3 commits intomainfrom
devin/1776277270-fix-menu-popover-padding

Conversation

@cpurps22
Copy link
Copy Markdown
Contributor

@cpurps22 cpurps22 commented Apr 15, 2026

Summary

When a Menu or ListBox is rendered inside a DialogTrigger popover, the popover's 8px padding (--lp-spacing-300) stacks on top of the menu's own 4px container padding (--lp-spacing-200), resulting in 12px of container padding instead of the expected 4px. This creates visually inconsistent menus compared to those rendered via MenuTrigger (which has no popover padding).

This change splits the DialogTrigger padding rule into two selectors:

  1. &[data-trigger='DialogTrigger']:not(:has([role='menu'], [role='listbox'])) — preserves padding for non-menu dialog content
  2. &[data-trigger='DialogTrigger']:has(input[type='search']) — re-adds padding when a search input is present (e.g. autocomplete/combobox patterns inside a DialogTrigger)

Plain menus/listboxes inside a DialogTrigger popover no longer receive the extra popover padding, while search-containing popovers retain it.

Human review checklist

  • Verify selector precedence: when both selectors could apply (a DialogTrigger popover with a listbox AND a search input), confirm the search selector wins and padding is applied. Since both are comma-separated in the same rule, either matching should apply the padding — but worth verifying in a real component.
  • Confirm no existing DialogTrigger popovers with mixed content (dialog elements alongside a menu, no search) are negatively impacted
  • Review Chromatic visual diffs to validate the padding change looks correct across all affected stories
  • A follow-up change in consumer repos (e.g. gonfalon) to use MenuTrigger instead of DialogTrigger for pure menu popovers would be the more architecturally correct long-term fix

Testing approaches

  • Visual regression via Chromatic CI
  • Manual verification: open any DialogTrigger popover containing a Menu and confirm padding matches a standard MenuTrigger menu (4px container padding, no extra outer padding from the popover)
  • Manual verification: open a DialogTrigger popover containing a search input + listbox and confirm padding is still present around the search field

Link to Devin session: https://app.devin.ai/sessions/e83a67884e96414b8f6b288b8c045e6d
Requested by: @cpurps22

…menu/listbox

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@devin-ai-integration
Copy link
Copy Markdown
Contributor

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Apr 15, 2026

🦋 Changeset detected

Latest commit: d00e544

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

This PR includes changesets to release 1 package
Name Type
@launchpad-ui/components 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

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new bot commented Apr 15, 2026

yarn add https://pkg.pr.new/@launchpad-ui/components@1884.tgz
yarn add https://pkg.pr.new/@launchpad-ui/icons@1884.tgz
yarn add https://pkg.pr.new/@launchpad-ui/tokens@1884.tgz

commit: d00e544

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
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.

1 participant