Skip to content

Fix/dropdown menu trigger - #8

Draft
JoaoTMDias wants to merge 8 commits into
michael-andreuzza:mainfrom
JoaoTMDias:fix/dropdown-menu-trigger
Draft

JoaoTMDias wants to merge 8 commits into
michael-andreuzza:mainfrom
JoaoTMDias:fix/dropdown-menu-trigger

Conversation

@JoaoTMDias

Copy link
Copy Markdown
Contributor

Summary

DropdownMenuTrigger now renders one native <button> instead of a focusable <span> wrapper.

This fixes incorrect button semantics, nested interactive controls, and inconsistent keyboard/focus behaviour.

Changes

  • Reuse the existing Button component API and icon slots.
  • Force type="button".
  • Keep dropdown-owned ARIA and data attributes controlled by the runtime.
  • Support native button activation, disabled, event.defaultPrevented, ArrowUp and ArrowDown.
  • Preserve Escape handling, item selection, submenu behaviour and focus restoration.
  • Migrate dropdown and breadcrumb examples away from nested Button composition.
  • Add button as a dropdown registry dependency.
  • Regenerate the public registry artefacts.
  • Add focused Playwright coverage.

Verification

  • npm run check ✅
  • npm run build ✅
  • Dropdown E2E tests: 4 passed ✅

Breaking change

Consumers using:

<DropdownMenuTrigger>
  <Button>Open menu</Button>
</DropdownMenuTrigger>

must migrate to

<DropdownMenuTrigger>Open menu</DropdownMenuTrigger>

Button props now belong directly on DropdownMenuTrigger.

Note: Issues are disabled in this repository, so this PR has no linked issue.

@JoaoTMDias

Copy link
Copy Markdown
Contributor Author

Hi @michael-andreuzza,

The initial Playwright setup is ready in this PR. It keeps the existing Astro checks, build, and CLI smoke tests unchanged, while adding focused browser coverage for component behaviour.

When you have time, could you please review the approach and let me know if you’d prefer any changes before I expand the coverage?

Thanks!

P.S. I also have a draft PR based on this one. It fixes an issue with the dropdown trigger and adds focused tests for that behaviour.

@michael-andreuzza

Copy link
Copy Markdown
Owner

The dropdown fix itself is great and I'd like to merge it. A single native button is the right call, and since components are installed as source, the API change is fine with a note in the changelog.

Since I'm not adding Playwright (see #7), could you rebase this onto main with only the dropdown changes? That means dropping the workflow, playwright.config.ts, tests/, the Playwright/axe dependencies, the .gitignore entries, the Logo.astro test id and the ComponentPreview/accordion id changes.

Also, please re-run npm run generate-registry and commit the result. public/registry/dropdown-menu.json is currently out of sync with DropdownMenuTrigger.astro.

Thanks!

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.

2 participants