feat(Avatar): support colorful#12460
Open
kmcfaul wants to merge 4 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (7)
📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds color variants, initials, and children support to Avatar with conditional div/img rendering; updates PatternFly dependency to 6.6.0-prerelease.8; includes tests and multiple example files demonstrating colorful SVGs, initials, and icons. ChangesAvatar colorful and nested content support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Collaborator
|
Preview: https://pf-react-pr-12460.surge.sh A11y report: https://pf-react-pr-12460-a11y.surge.sh |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/react-core/src/components/Avatar/__tests__/Avatar.test.tsx (1)
75-92: ⚡ Quick winAdd a regression test for colorful-over-bordered precedence.
Current additions validate colorful classes, but not the contract that bordered must be omitted when colorful is used.
Suggested test
+test('Does not apply bordered modifier when color is used', () => { + render( + <Avatar alt="avatar" color="red" isBordered> + Test + </Avatar> + ); + expect(screen.getByText('Test')).toHaveClass(styles.modifiers.colorful); + expect(screen.getByText('Test')).not.toHaveClass(styles.modifiers.bordered); +});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/react-core/src/components/Avatar/__tests__/Avatar.test.tsx` around lines 75 - 92, Add a regression test in Avatar.test.tsx that verifies the "colorful-over-bordered" precedence by rendering <Avatar alt="avatar" color="red" bordered> (or iterate colors with test.each) and asserting the rendered node has styles.modifiers.colorful (or styles.modifiers[color]) and does NOT have styles.modifiers.bordered; place the test near the existing color tests and use screen.getByText('Test') for assertions to match the current test patterns.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/react-core/src/components/Avatar/Avatar.tsx`:
- Around line 37-43: avatarClasses is built including isBordered unconditionally
which causes pf-m-bordered to persist for colorful avatars; update the logic so
bordered is only applied when color is not set. For example, keep the existing
avatarClasses for the non-colorful branch (use css(styles.avatar,
styles.modifiers[size], isBordered && styles.modifiers.bordered, className)) but
for the colorful branch compute a separate class list (e.g., baseColored =
css(styles.avatar, styles.modifiers[size], className) or reuse avatarClasses
without the bordered modifier) and use that when rendering the color path so
styles.modifiers.bordered is not included when color is truthy; reference
avatarClasses, styles.modifiers.bordered, isBordered and the color branch where
css(avatarClasses, color && styles.modifiers.colorful, color &&
styles.modifiers[color]) is used.
---
Nitpick comments:
In `@packages/react-core/src/components/Avatar/__tests__/Avatar.test.tsx`:
- Around line 75-92: Add a regression test in Avatar.test.tsx that verifies the
"colorful-over-bordered" precedence by rendering <Avatar alt="avatar"
color="red" bordered> (or iterate colors with test.each) and asserting the
rendered node has styles.modifiers.colorful (or styles.modifiers[color]) and
does NOT have styles.modifiers.bordered; place the test near the existing color
tests and use screen.getByText('Test') for assertions to match the current test
patterns.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 94ebea1f-1682-44bc-8b86-ef1a67268557
⛔ Files ignored due to path filters (2)
packages/react-core/src/components/assets/img_avatar-light.svgis excluded by!**/*.svgyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (15)
packages/react-core/package.jsonpackages/react-core/src/components/Avatar/Avatar.tsxpackages/react-core/src/components/Avatar/__tests__/Avatar.test.tsxpackages/react-core/src/components/Avatar/examples/Avatar.mdpackages/react-core/src/components/Avatar/examples/AvatarBasic.tsxpackages/react-core/src/components/Avatar/examples/AvatarBordered.tsxpackages/react-core/src/components/Avatar/examples/AvatarColorful.tsxpackages/react-core/src/components/Avatar/examples/AvatarIcons.tsxpackages/react-core/src/components/Avatar/examples/AvatarInitials.tsxpackages/react-core/src/components/Avatar/examples/AvatarSizeVariations.tsxpackages/react-core/src/components/Avatar/examples/avatar.csspackages/react-docs/package.jsonpackages/react-icons/package.jsonpackages/react-styles/package.jsonpackages/react-tokens/package.json
Comment on lines
+37
to
+43
| const avatarClasses = css(styles.avatar, styles.modifiers[size], isBordered && styles.modifiers.bordered, className); | ||
|
|
||
| if (initials || children) { | ||
| return ( | ||
| <div | ||
| className={css(avatarClasses, color && styles.modifiers.colorful, color && styles.modifiers[color])} | ||
| role="img" |
There was a problem hiding this comment.
Colorful avatars currently still keep pf-m-bordered when isBordered is true.
Line 37 builds avatarClasses with isBordered unconditionally, and Line 42 reuses it in the colorful path. That violates the documented behavior that colorful styling takes precedence over bordered.
Proposed fix
- const avatarClasses = css(styles.avatar, styles.modifiers[size], isBordered && styles.modifiers.bordered, className);
+ const baseAvatarClasses = css(styles.avatar, styles.modifiers[size], className);
if (initials || children) {
return (
<div
- className={css(avatarClasses, color && styles.modifiers.colorful, color && styles.modifiers[color])}
+ className={css(
+ baseAvatarClasses,
+ !color && isBordered && styles.modifiers.bordered,
+ color && styles.modifiers.colorful,
+ color && styles.modifiers[color]
+ )}
role="img"
aria-label={alt}
{...props}
>
@@
- return <img src={src} alt={alt} className={avatarClasses} {...props} />;
+ return <img src={src} alt={alt} className={css(baseAvatarClasses, isBordered && styles.modifiers.bordered)} {...props} />;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/react-core/src/components/Avatar/Avatar.tsx` around lines 37 - 43,
avatarClasses is built including isBordered unconditionally which causes
pf-m-bordered to persist for colorful avatars; update the logic so bordered is
only applied when color is not set. For example, keep the existing avatarClasses
for the non-colorful branch (use css(styles.avatar, styles.modifiers[size],
isBordered && styles.modifiers.bordered, className)) but for the colorful branch
compute a separate class list (e.g., baseColored = css(styles.avatar,
styles.modifiers[size], className) or reuse avatarClasses without the bordered
modifier) and use that when rendering the color path so
styles.modifiers.bordered is not included when color is truthy; reference
avatarClasses, styles.modifiers.bordered, isBordered and the color branch where
css(avatarClasses, color && styles.modifiers.colorful, color &&
styles.modifiers[color]) is used.
mcoker
reviewed
Jun 12, 2026
mcoker
left a comment
mcoker
left a comment
Contributor
There was a problem hiding this comment.
LGTM, just add isInline to the <Icon> components in the examples. The other comments are nits/questions.
| #ws-react-c-avatar-with-svgs, | ||
| #ws-react-c-avatar-with-initials, | ||
| #ws-react-c-avatar-with-icons { | ||
| display: flex; |
Contributor
There was a problem hiding this comment.
I'd also add flex-wrap: wrap so these wrap if there isn't enough space. An alternative is to remove the flex CSS and put spaces between the avatars in the examples, I think I've seen it done like {' '} before.
Contributor
Author
There was a problem hiding this comment.
Added the wrap, we have done {' '} before but sometimes prettier formats things oddly when that's used.
Comment on lines
+7
to
+9
| <Icon size="xl"> | ||
| <RhUiAiChatbotIcon /> | ||
| </Icon> |
Contributor
There was a problem hiding this comment.
We should probably review this with design, to see if using an icon as a child is a use case and if we need to change the icon size so it isn't clipped by the circle shape.
If you add isInline and leave the size it will fix the icon color
Contributor
Author
There was a problem hiding this comment.
Using an icon is the use case for chatbot. It may not need to be in an Icon but that's the easiest way to adjust the size that I could see.
Contributor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What: Closes #12442
color,initials, andchildrenprops toAvatarNotes:
altis a required prop forAvatar, repurposedaltto function asaria-labelfor children/initials structure (passingaria-labelas well asaltshould still function and overridealt). LMK if we'd rather keep the props separate and makealtoptional.isBordereddoesn't work with colorful (colorful has precedence over bordered modifier when both are applied). Should these modifiers be allowed to be used together?childrenorinitialswithoutcolorprop looks off. Should we still be applyingpf-m-colorfulfor the formatting without a color specified?Summary by CodeRabbit
Summary by CodeRabbit
New Features
Documentation
Tests
Chores