feat(avatar): add support for icons#8454
Merged
Merged
Conversation
Contributor
|
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:
WalkthroughThe Avatar component is refactored to conditionally render either a ChangesAvatar Template Refactor and Component Updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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-pr-8454.surge.sh A11y report: https://pf-pr-8454-a11y.surge.sh |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@src/patternfly/components/Avatar/avatar.hbs`:
- Around line 1-31: The logic flow is broken for both avatar--src and
avatar--IsColorful cases. In the div branch, the avatar--src condition renders a
hardcoded inline SVG instead of using the provided avatar--src URL as an image
source. Additionally, the avatar--IsColorful modifier is only applied to the div
branch but not to the img fallback, which means the colorful modifier is lost in
the fallback path. Fix this by replacing the hardcoded SVG at lines 18-22 with
an img tag that uses avatar--src as the src attribute, and restructure the
conditional logic so that the avatar--IsColorful modifier is applied to both the
div branch (for initials/icons/partial-block) and the img fallback branch.
Ensure both rendering paths properly handle the avatar--src URL and the colorful
styling.
In `@src/patternfly/components/Avatar/examples/Avatar.md`:
- Line 102: In the Avatar usage table at the specified location, the "Class"
column entry contains the template parameter `avatar--IsColorful=true`, which is
not a CSS class selector. Replace this entry with the actual emitted CSS class
(such as `.pf-m-colorful` or `.pf-v6-c-avatar.pf-m-colorful`) that results from
using that template parameter. If needed, document the template API parameter
usage separately outside the CSS class table to maintain clear separation
between template parameters and actual CSS selectors.
- Around line 73-83: The introductory sentence and all aria-label attributes in
the colorful avatar examples are incorrectly referencing "initial" when they
should reference "icon" since these examples use the rh-ui-ai-chatbot icon.
Update the sentence at the beginning to describe these as icon-based avatars,
and replace "initial" with "icon" in all nine avatar--aria-label attributes to
accurately reflect that assistive technology users are seeing an icon, not a
user initial.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 215ca12d-4ba5-4d43-9269-55b976da9a73
📒 Files selected for processing (2)
src/patternfly/components/Avatar/avatar.hbssrc/patternfly/components/Avatar/examples/Avatar.md
Comment on lines
+1
to
+31
| {{#ifAny avatar--initials avatar--icon @partial-block avatar--src}} | ||
| <div class="{{pfv}}avatar | ||
| {{setModifiers | ||
| avatar--IsColorful='pf-m-colorful' | ||
| avatar--modifier=avatar--modifier | ||
| }}" | ||
| {{#if avatar--attribute}} | ||
| {{{avatar--attribute}}} | ||
| {{/if}} | ||
| role="img" | ||
| {{#if avatar--aria-label}}aria-label="{{avatar--aria-label}}"{{else}}aria-hidden="true"{{/if}}> | ||
| {{#if avatar--initials}} | ||
| <span class="{{pfv}}avatar__initials" aria-hidden="true">{{avatar--initials}}</span> | ||
| {{else if avatar--icon}} | ||
| {{ pfIcon icon=avatar--icon}} | ||
| {{else if @partial-block}} | ||
| {{> @partial-block}} | ||
| {{else if avatar--src}} | ||
| <svg aria-hidden="true" data-name="Layer 1" xmlns="http://www.w3.org/2000/svg" version="1.1" viewBox="0 0 36 36"> | ||
| <path fill="currentColor" d="M17.7,20.1c-3.5,0-6.4-2.9-6.4-6.4s2.9-6.4,6.4-6.4,6.4,2.9,6.4,6.4-2.8,6.4-6.4,6.4Z"/> | ||
| <path fill="currentColor" d="M30.6,36c-.4-3.9-1.3-9-2.9-11-1.1-1.4-2.3-2.2-3.5-2.6s-1.8-.6-6.3-.6-6.1.7-6.1.7c-1.2.4-2.4,1.2-3.4,2.6-1.7,1.9-2.6,7.1-3,10.9h25.2Z"/> | ||
| </svg> | ||
| {{/if}} | ||
| </div> | ||
| {{else}} | ||
| <img class="{{pfv}}avatar | ||
| {{setModifiers | ||
| avatar--modifier=avatar--modifier | ||
| }}" | ||
| alt="{{#if avatar--alt}}{{avatar--alt}}{{else}}{{/if}}" | ||
| src="/assets/images/img_avatar-light.svg" |
Contributor
There was a problem hiding this comment.
avatar--src and avatar--IsColorful branching currently breaks both custom-image and colorful-default flows.
The ifAny gate uses avatar--src (Line 1), but the div branch never uses that URL as an image source and instead renders inline SVG (Lines 18-22). Meanwhile, the img fallback always uses the hardcoded default src (Line 31) and does not include the colorful modifier path. This conflicts with the updated examples and will render incorrect output for both avatar--src and avatar--IsColorful=true cases.
Proposed fix
-{{`#ifAny` avatar--initials avatar--icon `@partial-block` avatar--src}}
+{{`#ifAny` avatar--initials avatar--icon `@partial-block` avatar--IsColorful}}
<div class="{{pfv}}avatar
{{setModifiers
avatar--IsColorful='pf-m-colorful'
avatar--modifier=avatar--modifier
}}"
@@
- {{else if avatar--src}}
+ {{else}}
<svg aria-hidden="true" data-name="Layer 1" xmlns="http://www.w3.org/2000/svg" version="1.1" viewBox="0 0 36 36">
<path fill="currentColor" d="M17.7,20.1c-3.5,0-6.4-2.9-6.4-6.4s2.9-6.4,6.4-6.4,6.4,2.9,6.4,6.4-2.8,6.4-6.4,6.4Z"/>
<path fill="currentColor" d="M30.6,36c-.4-3.9-1.3-9-2.9-11-1.1-1.4-2.3-2.2-3.5-2.6s-1.8-.6-6.3-.6-6.1.7-6.1.7c-1.2.4-2.4,1.2-3.4,2.6-1.7,1.9-2.6,7.1-3,10.9h25.2Z"/>
</svg>
@@
- src="/assets/images/img_avatar-light.svg"
+ src="{{`#if` avatar--src}}{{avatar--src}}{{else}}/assets/images/img_avatar-light.svg{{/if}}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| {{#ifAny avatar--initials avatar--icon @partial-block avatar--src}} | |
| <div class="{{pfv}}avatar | |
| {{setModifiers | |
| avatar--IsColorful='pf-m-colorful' | |
| avatar--modifier=avatar--modifier | |
| }}" | |
| {{#if avatar--attribute}} | |
| {{{avatar--attribute}}} | |
| {{/if}} | |
| role="img" | |
| {{#if avatar--aria-label}}aria-label="{{avatar--aria-label}}"{{else}}aria-hidden="true"{{/if}}> | |
| {{#if avatar--initials}} | |
| <span class="{{pfv}}avatar__initials" aria-hidden="true">{{avatar--initials}}</span> | |
| {{else if avatar--icon}} | |
| {{ pfIcon icon=avatar--icon}} | |
| {{else if @partial-block}} | |
| {{> @partial-block}} | |
| {{else if avatar--src}} | |
| <svg aria-hidden="true" data-name="Layer 1" xmlns="http://www.w3.org/2000/svg" version="1.1" viewBox="0 0 36 36"> | |
| <path fill="currentColor" d="M17.7,20.1c-3.5,0-6.4-2.9-6.4-6.4s2.9-6.4,6.4-6.4,6.4,2.9,6.4,6.4-2.8,6.4-6.4,6.4Z"/> | |
| <path fill="currentColor" d="M30.6,36c-.4-3.9-1.3-9-2.9-11-1.1-1.4-2.3-2.2-3.5-2.6s-1.8-.6-6.3-.6-6.1.7-6.1.7c-1.2.4-2.4,1.2-3.4,2.6-1.7,1.9-2.6,7.1-3,10.9h25.2Z"/> | |
| </svg> | |
| {{/if}} | |
| </div> | |
| {{else}} | |
| <img class="{{pfv}}avatar | |
| {{setModifiers | |
| avatar--modifier=avatar--modifier | |
| }}" | |
| alt="{{#if avatar--alt}}{{avatar--alt}}{{else}}{{/if}}" | |
| src="/assets/images/img_avatar-light.svg" | |
| {{`#ifAny` avatar--initials avatar--icon `@partial-block` avatar--IsColorful}} | |
| <div class="{{pfv}}avatar | |
| {{setModifiers | |
| avatar--IsColorful='pf-m-colorful' | |
| avatar--modifier=avatar--modifier | |
| }}" | |
| {{`#if` avatar--attribute}} | |
| {{{avatar--attribute}}} | |
| {{/if}} | |
| role="img" | |
| {{`#if` avatar--aria-label}}aria-label="{{avatar--aria-label}}"{{else}}aria-hidden="true"{{/if}}> | |
| {{`#if` avatar--initials}} | |
| <span class="{{pfv}}avatar__initials" aria-hidden="true">{{avatar--initials}}</span> | |
| {{else if avatar--icon}} | |
| {{ pfIcon icon=avatar--icon}} | |
| {{else if `@partial-block`}} | |
| {{> `@partial-block`}} | |
| {{else}} | |
| <svg aria-hidden="true" data-name="Layer 1" xmlns="http://www.w3.org/2000/svg" version="1.1" viewBox="0 0 36 36"> | |
| <path fill="currentColor" d="M17.7,20.1c-3.5,0-6.4-2.9-6.4-6.4s2.9-6.4,6.4-6.4,6.4,2.9,6.4,6.4-2.8,6.4-6.4,6.4Z"/> | |
| <path fill="currentColor" d="M30.6,36c-.4-3.9-1.3-9-2.9-11-1.1-1.4-2.3-2.2-3.5-2.6s-1.8-.6-6.3-.6-6.1.7-6.1.7c-1.2.4-2.4,1.2-3.4,2.6-1.7,1.9-2.6,7.1-3,10.9h25.2Z"/> | |
| </svg> | |
| {{/if}} | |
| </div> | |
| {{else}} | |
| <img class="{{pfv}}avatar | |
| {{setModifiers | |
| avatar--modifier=avatar--modifier | |
| }}" | |
| alt="{{`#if` avatar--alt}}{{avatar--alt}}{{else}}{{/if}}" | |
| src="{{`#if` avatar--src}}{{avatar--src}}{{else}}/assets/images/img_avatar-light.svg{{/if}}" |
🤖 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 `@src/patternfly/components/Avatar/avatar.hbs` around lines 1 - 31, The logic
flow is broken for both avatar--src and avatar--IsColorful cases. In the div
branch, the avatar--src condition renders a hardcoded inline SVG instead of
using the provided avatar--src URL as an image source. Additionally, the
avatar--IsColorful modifier is only applied to the div branch but not to the img
fallback, which means the colorful modifier is lost in the fallback path. Fix
this by replacing the hardcoded SVG at lines 18-22 with an img tag that uses
avatar--src as the src attribute, and restructure the conditional logic so that
the avatar--IsColorful modifier is applied to both the div branch (for
initials/icons/partial-block) and the img fallback branch. Ensure both rendering
paths properly handle the avatar--src URL and the colorful styling.
Contributor
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 `@src/patternfly/components/MenuToggle/examples/MenuToggle.md`:
- Line 293: The `avatar--IsImg` parameter in the handlebars template helper is
being passed as a bare positional token instead of as an explicit hash
parameter, causing it to evaluate as undefined and fail the conditional check in
the Avatar component. Change all three occurrences of the handlebars helper
invocations to use `avatar--IsImg=true` instead of `avatar--IsImg` as a
positional token (at lines 293, 307, and 321 in the MenuToggle.md file) to
explicitly set the hash parameter and ensure the avatar renders as an image
rather than falling back to the initials/icon branch.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 79002174-2c43-46ca-a85b-67bb3ddfdf0a
📒 Files selected for processing (5)
src/patternfly/components/Avatar/avatar--colorful.hbssrc/patternfly/components/Avatar/avatar.hbssrc/patternfly/components/Avatar/examples/Avatar.mdsrc/patternfly/components/MenuToggle/examples/MenuToggle.mdsrc/patternfly/demos/Compass/compass--header.hbs
💤 Files with no reviewable changes (1)
- src/patternfly/components/Avatar/avatar--colorful.hbs
✅ Files skipped from review due to trivial changes (1)
- src/patternfly/demos/Compass/compass--header.hbs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/patternfly/components/Avatar/examples/Avatar.md
mcoker
force-pushed
the
avatar-icon-support
branch
from
07d8733 to
500494a
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/patternfly/components/Avatar/examples/Avatar.md (1)
39-44: 💤 Low valueRemove extraneous wrapper properties that don't cascade to child partials.
The
avatar--IsColorful=trueandavatar--IsRed=trueproperties on the wrapper block partial don't propagate to the childavatarpartials inside; each avatar explicitly specifies its own size and properties. The wrapper properties are dead code and should be removed for clarity.🧹 Proposed cleanup
-{{#> wrapper avatar--IsColorful=true avatar--IsRed=true}} +{{#> wrapper}} {{> avatar avatar--IsSmall=true avatar--initials="C" avatar--aria-label="Small red avatar with initial C"}} {{> avatar avatar--IsMedium=true avatar--initials="C" avatar--aria-label="Medium red avatar with initial C"}} {{> avatar avatar--IsLarge=true avatar--initials="C" avatar--aria-label="Large red avatar with initial C"}} {{> avatar avatar--IsXLarge=true avatar--initials="C" avatar--aria-label="Extra large red avatar with initial C"}} {{/wrapper}}🤖 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 `@src/patternfly/components/Avatar/examples/Avatar.md` around lines 39 - 44, The wrapper block in the Avatar example contains extraneous properties avatar--IsColorful=true and avatar--IsRed=true that do not cascade to the child avatar partials and are therefore dead code. Remove these unused properties from the wrapper opening tag while keeping the wrapper itself intact, since the child avatar partials already explicitly specify all their own size and styling properties through their individual attributes.
🤖 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 `@scripts/helpers.mjs`:
- Around line 438-444: The path resolution logic in the conditional block
(checking if filePath starts with '/') is vulnerable to path traversal attacks
because path.join() normalizes but doesn't prevent escape sequences like '..'.
After constructing the resolvedPath using path.join() in both branches, add
validation to ensure the final resolved path stays within its intended directory
boundary. For the first branch where filePath starts with '/', verify the
resolvedPath stays within the 'src/patternfly' directory. For the second branch,
verify it stays within projectRoot. Use path.relative() and check that the
relative path doesn't start with '..' to confirm the resolved path hasn't
escaped the intended boundary, similar to the defensive pattern already used in
the pfIcon helper.
In `@src/patternfly/components/Avatar/examples/Avatar.md`:
- Line 14: Fix the typo in the avatar component attribute where
`avatar--arial-label` is incorrectly spelled. Change it to `avatar--aria-label`
to ensure the accessibility label attribute is properly recognized and applied
to the avatar component.
---
Nitpick comments:
In `@src/patternfly/components/Avatar/examples/Avatar.md`:
- Around line 39-44: The wrapper block in the Avatar example contains extraneous
properties avatar--IsColorful=true and avatar--IsRed=true that do not cascade to
the child avatar partials and are therefore dead code. Remove these unused
properties from the wrapper opening tag while keeping the wrapper itself intact,
since the child avatar partials already explicitly specify all their own size
and styling properties through their individual attributes.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 49de3efd-0446-4de7-b86c-130c4d3906ed
⛔ Files ignored due to path filters (1)
src/patternfly/assets/images/pf-logo-avatar.jpgis excluded by!**/*.jpg
📒 Files selected for processing (5)
scripts/helpers.mjssrc/patternfly/components/Avatar/avatar--colors.hbssrc/patternfly/components/Avatar/avatar.hbssrc/patternfly/components/Avatar/examples/Avatar.csssrc/patternfly/components/Avatar/examples/Avatar.md
✅ Files skipped from review due to trivial changes (2)
- src/patternfly/components/Avatar/examples/Avatar.css
- src/patternfly/components/Avatar/avatar--colors.hbs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/patternfly/components/Avatar/avatar.hbs
thatblindgeye
approved these changes
Jun 18, 2026
thatblindgeye
left a comment
thatblindgeye
left a comment
Contributor
There was a problem hiding this comment.
LGTM. Wondering if the suggestion from coderabbit above would be worth including. Alternatively, if the new helper's purpose is really just scoped to using SVG files inline in core, we could just instead manually do it by copy+pasting the actual svg code into examples (right click the svg file > "open with" > "Text editor").
I guess it's a question of whether doing that is more or less time consuming/tedious/etc than having to pass in the file path to the svg.
jcmill
approved these changes
Jun 18, 2026
jcmill
left a comment
jcmill
left a comment
Contributor
There was a problem hiding this comment.
Looks great! Just a few nits of trailing spaces. 👤
Contributor
Author
@thatblindgeye yeah, that's what we do currently. It's fine if we use an SVG/image once, but if we use it multiple times especially across multiple components/examples, it's not ideal. We then either need to put the icon in a handlebars file so we can reference it in different examples (but we can't use that in |
Co-authored-by: Jake Miller <jake83miller@gmail.com>
Collaborator
|
🎉 This PR is included in version 6.6.0-prerelease.9 🎉 The release is available on: Your semantic-release bot 📦🚀 |
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.
fixes #8456
Summary by CodeRabbit