Fix tooltip in AvatarStack
#3676
Conversation
🦋 Changeset detectedLatest commit: d6e09a3 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 |
| ) | ||
|
|
||
| @body_arguments[:tabindex] = tooltipped ? 0 : nil | ||
| @body_arguments[:id] = tooltipped ? @body_arguments[:id] ||= self.class.generate_id : nil |
There was a problem hiding this comment.
is this deleting the supplied id if not tooltipped? 🤔
There was a problem hiding this comment.
Good call! We can add @body_arguments[:id] and the end of the ternary instead.
There was a problem hiding this comment.
Pull Request Overview
This PR modernizes the tooltip implementation in the AvatarStack component by replacing the deprecated Primer::Tooltip with the modern Primer::Alpha::Tooltip component.
Key changes:
- Refactored tooltip logic to use
Primer::Alpha::Tooltipinstead of the deprecated tooltip wrapper - Updated component initialization to properly configure tooltip arguments and accessibility attributes
- Modified the component template to render the tooltip as a separate component
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| app/components/primer/beta/avatar_stack.rb | Refactored tooltip implementation to use modern Alpha::Tooltip component |
| app/components/primer/beta/avatar_stack.html.erb | Added conditional rendering of the new tooltip component |
| test/components/beta/avatar_stack_test.rb | Updated test selector to remove deprecated tooltip class reference |
| previews/primer/beta/avatar_stack_preview.rb | Added href attributes to avatar examples for testing tooltip functionality |
| .changeset/blue-phones-boil.md | Added changeset documentation for the tooltip modernization |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| @body_arguments[:aria] ||= {} | ||
| @body_arguments[:aria][:label] = tooltipped && @body_arguments[:label].present? ? @body_arguments[:label] : nil | ||
|
|
||
|
|
There was a problem hiding this comment.
[nitpick] Remove the extra blank line to maintain consistent code formatting.
Copilot uses AI. Check for mistakes.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Authors: Please fill out this form carefully and completely.
Reviewers: By approving this Pull Request you are approving the code change, as well as its deployment and mitigation plans.
Please read this description carefully. If you feel there is anything unclear or missing, please ask for updates.
What are you trying to accomplish?
Addresses https://github.com/github/accessibility-audits/issues/12705
Ensures
AvatarStackis utilizing modernTooltipcomponentIntegration
No
List the issues that this change affects.
https://github.com/github/accessibility-audits/issues/12705
Risk Assessment
What approach did you choose and why?
Replaces old CSS
Tooltipwith PVC `Tooltip.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.