-
Notifications
You must be signed in to change notification settings - Fork 72
[LG-5686] fix(avatar): use unique gradient IDs in AssistantAvatar #3373
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
Conversation
|
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 fixes a bug where multiple AssistantAvatar components on the same page would have conflicting SVG gradient IDs, causing incorrect rendering. The fix replaces a hardcoded gradient ID constant with dynamic IDs generated using the useIdAllocator hook.
Key Changes:
- Replaced hardcoded
GRADIENT_IDconstant with dynamic gradient IDs usinguseIdAllocator - Added
@leafygreen-ui/hooksas a dependency to the avatar package
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/avatar/src/AssistantAvatar/AssistantAvatar.tsx | Removed hardcoded gradient ID constant and implemented useIdAllocator hook to generate unique IDs per component instance |
| packages/avatar/package.json | Added @leafygreen-ui/hooks workspace dependency |
| packages/avatar/tsconfig.json | Added TypeScript project reference to the hooks package |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
|
Size Change: +82 B (0%) Total Size: 1.81 MB
ℹ️ View Unchanged
|
| }: AssistantAvatarProps) => { | ||
| const { darkMode, theme } = useDarkMode(darkModeProp); | ||
| const fill = disabled ? getDisabledFill(theme) : `url(#${GRADIENT_ID})`; | ||
| const gradientId = useIdAllocator({ |
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.
Do we need a changeset for this?
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.
Just realized that there is already a changeset. Approving!
✍️ Proposed changes
This PR updates
AssistantAvatarto useuseIdAllocatorfrom@leafygreen-ui/hooksto generate unique gradient IDs instead of a hardcoded constant. This prevents ID collisions when multipleAssistantAvatarcomponents are rendered on the same page, which could cause incorrect gradient rendering.🎟️ Jira ticket: LG-5686
✅ Checklist
pnpm changesetand documented my changes🧪 How to test changes
AssistantAvatarcomponents on the same page (e.g., in a Storybook story)