feat(Avatar): support colorful#12460
Open
kmcfaul wants to merge 6 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Walkthrough
ChangesAvatar color, initials, and children support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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
thatblindgeye
force-pushed
the
avatar-colorful
branch
from
25e2682 to
9098897
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/examples/Avatar.md`:
- Line 53: Fix the spelling error in the Avatar.md file where "guildelines" is
misspelled. Change "guildelines" to "guidelines" in the sentence about the
avatar children example to ensure the documentation text is correctly spelled.
🪄 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: 0d99ee55-b42f-4bf7-a18c-afaec10c4cbf
⛔ Files ignored due to path filters (9)
packages/react-charts/src/victory/components/ChartContainer/__snapshots__/ChartContainer.test.tsx.snapis excluded by!**/*.snappackages/react-charts/src/victory/components/ChartDonut/__snapshots__/ChartDonut.test.tsx.snapis excluded by!**/*.snappackages/react-charts/src/victory/components/ChartDonutUtilization/__snapshots__/ChartDonutThreshold.test.tsx.snapis excluded by!**/*.snappackages/react-charts/src/victory/components/ChartDonutUtilization/__snapshots__/ChartDonutUtilization.test.tsx.snapis excluded by!**/*.snappackages/react-charts/src/victory/components/ChartLegend/__snapshots__/ChartLegend.test.tsx.snapis excluded by!**/*.snappackages/react-charts/src/victory/components/ChartLegendTooltip/__snapshots__/ChartLegendTooltip.test.tsx.snapis excluded by!**/*.snappackages/react-charts/src/victory/components/ChartPoint/__snapshots__/ChartPoint.test.tsx.snapis excluded by!**/*.snappackages/react-core/src/components/assets/img_avatar-light.svgis excluded by!**/*.svgyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (14)
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/AvatarInitials.tsxpackages/react-core/src/components/Avatar/examples/AvatarSizeVariations.tsxpackages/react-core/src/components/Avatar/examples/AvatarWithChildren.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
✅ Files skipped from review due to trivial changes (2)
- packages/react-styles/package.json
- packages/react-core/src/components/Avatar/examples/avatar.css
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/react-tokens/package.json
- packages/react-docs/package.json
- packages/react-icons/package.json
- packages/react-core/package.json
- packages/react-core/src/components/Avatar/tests/Avatar.test.tsx
- packages/react-core/src/components/Avatar/Avatar.tsx
- packages/react-core/src/components/Avatar/examples/AvatarSizeVariations.tsx
thatblindgeye
force-pushed
the
avatar-colorful
branch
from
9098897 to
a9a8e7c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/examples/Avatar.md`:
- Around line 17-33: The documentation for Avatar examples with children and
initials does not clearly explain that the alt property remains required for
accessibility. Add clarification in the text before or after the AvatarBasic.tsx
and AvatarColorModifiers.tsx examples (in the sections describing Pass in the
src property and With initials) to explicitly state that alt is required for all
Avatar variants and that it serves as the accessible name (becoming the
aria-label) when the Avatar is rendered with children or initials instead of an
image. This ensures users understand that alt cannot be omitted for non-image
variants.
🪄 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: 1ef713cd-6627-4e99-8f19-e8f5d7e01c85
⛔ Files ignored due to path filters (10)
packages/react-charts/src/victory/components/ChartContainer/__snapshots__/ChartContainer.test.tsx.snapis excluded by!**/*.snappackages/react-charts/src/victory/components/ChartDonut/__snapshots__/ChartDonut.test.tsx.snapis excluded by!**/*.snappackages/react-charts/src/victory/components/ChartDonutUtilization/__snapshots__/ChartDonutThreshold.test.tsx.snapis excluded by!**/*.snappackages/react-charts/src/victory/components/ChartDonutUtilization/__snapshots__/ChartDonutUtilization.test.tsx.snapis excluded by!**/*.snappackages/react-charts/src/victory/components/ChartLegend/__snapshots__/ChartLegend.test.tsx.snapis excluded by!**/*.snappackages/react-charts/src/victory/components/ChartLegendTooltip/__snapshots__/ChartLegendTooltip.test.tsx.snapis excluded by!**/*.snappackages/react-charts/src/victory/components/ChartPoint/__snapshots__/ChartPoint.test.tsx.snapis excluded by!**/*.snappackages/react-core/src/components/assets/img_avatar-light.svgis excluded by!**/*.svgpackages/react-core/src/components/assets/rhds.svgis excluded by!**/*.svgyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (14)
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/AvatarColorModifiers.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
✅ Files skipped from review due to trivial changes (3)
- packages/react-styles/package.json
- packages/react-docs/package.json
- packages/react-core/src/components/Avatar/examples/AvatarBasic.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/react-tokens/package.json
- packages/react-icons/package.json
- packages/react-core/src/components/Avatar/tests/Avatar.test.tsx
- packages/react-core/package.json
- packages/react-core/src/components/Avatar/Avatar.tsx
Comment on lines
+17
to
+33
| Pass in the `src` property to apply an image for the avatar, or pass `children` for custom content such as icons or svg elements. | ||
|
|
||
| ```ts file="./AvatarBasic.tsx" | ||
|
|
||
| ``` | ||
|
|
||
| ### Color modifiers | ||
|
|
||
| Color modifiers add visual interest and automatically include a border. The available colors are: "red", "orangered", "orange", "yellow", "green", "teal", "blue", "purple", and "gray". | ||
|
|
||
| ```ts file="./AvatarColorModifiers.tsx" | ||
|
|
||
| ``` | ||
|
|
||
| ### With initials | ||
|
|
||
| An avatar can display initials by using the `initials` property. It's recommended to pass only a single initial to ensure the text fits inside of the avatar. |
There was a problem hiding this comment.
Call out the alt/aria-label contract here.
The new children and initials examples should say that alt is still required for Avatar and becomes the accessible name unless aria-label is provided. Right now the docs read like alt can be omitted for the non-image variants.
Suggested text
- Pass in the `src` property to apply an image for the avatar, or pass `children` for custom content such as icons or svg elements.
+ Pass in the `src` property to apply an image for the avatar. When using `children` or `initials`, keep `alt` for the accessible name unless `aria-label` is provided.Based on PR objectives: alt remains required but is repurposed to act as aria-label when Avatar is rendered with children or initials.
🧰 Tools
🪛 LanguageTool
[style] ~33-~33: This phrase is redundant. Consider using “inside”.
Context: ... single initial to ensure the text fits inside of the avatar. ```ts file="./AvatarInitia...
(OUTSIDE_OF)
🤖 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/examples/Avatar.md` around lines 17
- 33, The documentation for Avatar examples with children and initials does not
clearly explain that the alt property remains required for accessibility. Add
clarification in the text before or after the AvatarBasic.tsx and
AvatarColorModifiers.tsx examples (in the sections describing Pass in the src
property and With initials) to explicitly state that alt is required for all
Avatar variants and that it serves as the accessible name (becoming the
aria-label) when the Avatar is rendered with children or initials instead of an
image. This ensures users understand that alt cannot be omitted for non-image
variants.
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
4 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
New Features
Documentation
Chores