Support dynamic labels for multiple variant SelectPanel#3825
Support dynamic labels for multiple variant SelectPanel#3825myabc wants to merge 2 commits intoprimer:mainfrom
SelectPanel#3825Conversation
🦋 Changeset detectedLatest commit: b084a5a 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 |
myabc
force-pushed
the
feature/select-panel-dynamic-label-multiple
branch
2 times, most recently
from
35d5564 to
ef8163d
Compare
myabc
changed the title
SelectPanel
There was a problem hiding this comment.
Pull request overview
This PR adds support for dynamic labels in SelectPanel's multiple select variant. Previously, dynamic labels only worked with the single select variant. Now both single and multiple variants can display selected item labels dynamically in the panel invoker button.
Key Changes:
- Extended dynamic label functionality to support multiple select variant, displaying selected items as comma-separated values
- Updated preview components to accept a
select_variantparameter for testing both single and multiple variants - Added comprehensive test coverage for both single and multiple select variants with dynamic labels
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| app/components/primer/alpha/select_panel_element.ts | Added logic to handle dynamic labels for multiple select variant by collecting and joining labels from all selected items |
| previews/primer/alpha/select_panel_preview.rb | Added select_variant parameter to dynamic label preview methods to support testing both variants |
| previews/primer/alpha/select_panel_preview/with_dynamic_label.html.erb | Changed hardcoded :single to parameterized select_variant |
| previews/primer/alpha/select_panel_preview/with_dynamic_label_and_aria_prefix.html.erb | Changed hardcoded :single to parameterized select_variant |
| test/system/alpha/select_panel_test.rb | Added four new test cases covering dynamic labels for both single and multiple variants, with and without aria prefix; cleaned up trailing whitespace |
| .changeset/afraid-monkeys-unite.md | Added changeset documenting this minor feature addition |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| itemLabel = this.querySelector(`[${this.ariaSelectionType}=true] .ActionListItem-label`)?.textContent | ||
| } else if (this.selectVariant === 'multiple') { | ||
| itemLabel = Array.from(this.querySelectorAll(`[${this.ariaSelectionType}=true] .ActionListItem-label`)) | ||
| .map(label => label.textContent.trim()) |
There was a problem hiding this comment.
The code assumes label.textContent is not null when calling .trim(). Although textContent typically returns an empty string rather than null, it would be safer to add null checking (e.g., label.textContent?.trim()) to handle edge cases consistently with line 956.
| .map(label => label.textContent.trim()) | |
| .map(label => label.textContent?.trim() ?? '') |
| itemLabel = this.querySelector(`[${this.ariaSelectionType}=true] .ActionListItem-label`)?.textContent | ||
| } else if (this.selectVariant === 'multiple') { | ||
| itemLabel = Array.from(this.querySelectorAll(`[${this.ariaSelectionType}=true] .ActionListItem-label`)) | ||
| .map(label => label.textContent.trim()) |
There was a problem hiding this comment.
If any label has null or empty textContent, the resulting string could include empty entries separated by commas (e.g., "Item 1, , Item 3"). Consider filtering out null or empty values before joining to avoid displaying empty entries in the label.
| .map(label => label.textContent.trim()) | |
| .map(label => label.textContent?.trim() || '') | |
| .filter(labelText => labelText.length > 0) |
| itemLabel = this.querySelector(`[${this.ariaSelectionType}=true] .ActionListItem-label`)?.textContent | ||
| } else if (this.selectVariant === 'multiple') { | ||
| itemLabel = Array.from(this.querySelectorAll(`[${this.ariaSelectionType}=true] .ActionListItem-label`)) | ||
| .map(label => label.textContent.trim()) | ||
| .join(', ') |
There was a problem hiding this comment.
The code doesn't call .trim() on the textContent for single select variant (line 956), but does call it for the multiple select variant (line 959). This inconsistency could lead to differences in whitespace handling between the two variants. Consider applying .trim() consistently in both cases.
myabc
force-pushed
the
feature/select-panel-dynamic-label-multiple
branch
from
54dcba6 to
df138f2
Compare
|
Hi! Happy holidays! I'm not the right person to review this, so I'm going to assign it to @jonrohan to have a look when he's back from the holidays. |
myabc
force-pushed
the
feature/select-panel-dynamic-label-multiple
branch
from
df138f2 to
b084a5a
Compare
2 participants
What are you trying to accomplish?
Screenshots
Integration
List the issues that this change affects.
Closes #3824
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.