-
Notifications
You must be signed in to change notification settings - Fork 129
Support dynamic labels for multiple variant ActionMenu
#3827
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 4de98c3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
ActionMenu
There was a problem hiding this 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()) |
Copilot
AI
Dec 26, 2025
There was a problem hiding this comment.
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.
| .map(label => label.textContent.trim()) | |
| .map(label => label.textContent?.trim() ?? '') |
cd4e13c to
4de98c3
Compare
What are you trying to accomplish?
Screenshots
Integration
List the issues that this change affects.
Closes #3826
Risk Assessment
What approach did you choose and why?
Anything you want to highlight for special attention from reviewers?
Accessibility
Merge checklist
Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.