Skip to content

Conversation

@myabc
Copy link
Contributor

@myabc myabc commented Dec 26, 2025

What are you trying to accomplish?

Screenshots

image

Integration

List the issues that this change affects.

Closes #3826

Risk Assessment

  • Low risk the change is small, highly observable, and easily rolled back.
  • Medium risk changes that are isolated, reduced in scope or could impact few users. The change will not impact library availability.
  • High risk changes are those that could impact customers and SLOs, low or no test coverage, low observability, or slow to rollback.

What approach did you choose and why?

Anything you want to highlight for special attention from reviewers?

Accessibility

  • Fixes axe scan violation - This change fixes an existing axe scan violation.
  • No new axe scan violation - This change does not introduce any new axe scan violations.
  • New axe violation - This change introduces a new axe scan violation. Please describe why the violation cannot be resolved below.

Merge checklist

  • Added/updated tests
  • Added/updated documentation
  • Added/updated previews (Lookbook)
  • Tested in Chrome
  • Tested in Firefox
  • Tested in Safari
  • Tested in Edge

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

@changeset-bot
Copy link

changeset-bot bot commented Dec 26, 2025

🦋 Changeset detected

Latest commit: 4de98c3

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

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

@myabc myabc changed the title Feature/action menu dynamic menu multiple Support dynamic labels for multiple variant ActionMenu Dec 26, 2025
@myabc myabc marked this pull request as ready for review December 26, 2025 02:14
Copilot AI review requested due to automatic review settings December 26, 2025 02:14
@myabc myabc requested a review from a team as a code owner December 26, 2025 02:14
@myabc myabc requested a review from TylerJDev December 26, 2025 02:14
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR extends the ActionMenu component to support dynamic labels for the multiple select variant. Previously, dynamic labels only worked for single select menus; now they also update dynamically when multiple items are selected in a multi-select menu.

Key changes:

  • Extended #setDynamicLabel() method to handle multiple select variant with comma-separated labels
  • Added space after the dynamic label prefix for better formatting
  • Added test coverage for the new multiple select dynamic label functionality

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
app/components/primer/alpha/action_menu/action_menu_element.ts Extended dynamic label functionality to support multiple select variant by removing single-select restriction and adding logic to join multiple selected item labels
test/system/alpha/action_menu_test.rb Renamed existing test for clarity and added new test to verify dynamic labels work correctly for multiple select variant
.changeset/sweet-candies-pay.md Added changeset entry documenting the new feature

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

itemLabel = this.querySelector('[aria-checked=true] .ActionListItem-label')?.textContent
} else if (this.selectVariant === 'multiple') {
itemLabel = Array.from(this.querySelectorAll(`[aria-checked=true] .ActionListItem-label`))
.map(label => label.textContent.trim())
Copy link

Copilot AI Dec 26, 2025

Choose a reason for hiding this comment

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

The textContent property can return null, which will cause a runtime error when calling .trim() on it. This should be handled to prevent the error. Consider using optional chaining or adding a null check before calling trim.

Suggested change
.map(label => label.textContent.trim())
.map(label => label.textContent?.trim() ?? '')

Copilot uses AI. Check for mistakes.
@myabc myabc force-pushed the feature/action-menu-dynamic-menu-multiple branch from cd4e13c to 4de98c3 Compare January 6, 2026 20:07
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.

ActionMenu multiple variant does not support dynamic labels

1 participant