-
Notifications
You must be signed in to change notification settings - Fork 1
bug/69230 Extend Primer::Beta::Avatar as Primer::OpenProject::AvatarWithFallback to render initials in a styled SVG when avatar img src is nil/blank
#387
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
🦋 Changeset detectedLatest commit: 4682247 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 |
b6c96c5 to
fe4a265
Compare
|
c5dc1a2 to
32c6147
Compare
…n avatar imag src is nil/blank
32c6147 to
9f3c2aa
Compare
|
Failing test is unrelated, also failing on main. |
2bda67d to
f65158c
Compare
The hash function is not consistently portable between JS/Ruby due to different "integer overflow handling (!?)". JS based coloring establishes consistency while we have both primer legacy (angular) component rendering avatar fallback
f65158c to
fa44e92
Compare
brunopagno
left a comment
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.
From where I stand it looks great! 👏
I also like that we don't depend on user or any data model to make it nice to use ❤️. I wish we could do this for most/all UI components.
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 enhances the Primer::Beta::Avatar component to render initials in a styled SVG when the avatar image source is nil or blank, addressing a bug in OpenProject where avatars in AvatarStack didn't render properly without an image source. The implementation uses SVG data URIs as fallbacks while maintaining semantic HTML structure and accessibility.
Key Changes:
- Makes
srcparameter optional in Avatar constructor (breaking change) - Adds SVG fallback generation with user initials extracted from
alttext - Implements client-side color correction via Catalyst custom element
- Adds comprehensive test coverage for the new fallback functionality
Reviewed changes
Copilot reviewed 13 out of 21 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
app/components/primer/beta/avatar.rb |
Modified Avatar component to support optional src, added SVG generation with initials, color hashing, and validation logic |
app/components/primer/beta/avatar_fallback.ts |
New Catalyst controller for client-side SVG color correction to match OpenProject's color generation |
app/components/primer/primer.ts |
Added import for new avatar_fallback TypeScript module |
test/components/beta/avatar_test.rb |
Added 14 comprehensive tests covering fallback rendering, initials generation, color consistency, shapes, and edge cases |
previews/primer/beta/avatar_preview.rb |
Added 6 new Lookbook previews demonstrating fallback functionality |
previews/primer/beta/avatar_stack_preview.rb |
Added 2 new previews showing fallback avatars in stacks |
previews/primer/beta/avatar_preview/*.html.erb |
New ERB templates for fallback preview rendering |
.changeset/heavy-donuts-bow.md |
Added changeset describing the enhancement (incorrectly marked as minor) |
.playwright/screenshots/* |
New visual regression test snapshots for fallback avatars |
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.
I'm seeing flickering behaviour when fallback avatars are rendered in an Avatar Stack.
Screen.Recording.2025-12-10.at.14.07.25.mov
http://localhost:4000/lookbook/inspect/primer/beta/avatar_stack/more_options
Chrome 143.0.7499.41 (Official Build) (arm64)
macOS Tahoe 26.1 (25B78)
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.
Sorry for the delay in giving feedback... 🙏🏻
General feedback - Part 1
Even though upstream/GitHub PVC is less of a moving target than before, I still think we need to set a very high bar when it comes to modifying upstream components directly.
In my opinion, we should only modify Primer::Alpha/Primer::Beta components directly when:
- EITHER the behaviour changes are generic and/or the fixes are of potential use to other library consumers and there is a likelihood we can upstream.
- OR any API changes are purely additive / very limited.
I don't think this change meets the above criteria. Instead, I think we should prefer Extension Over Mutation.
Instead of modifying Primer::Beta::Avatar directly, could we not introduce a Primer::OpenProject::Avatar or AvatarWithFallback component that decorates or is composed of a Primer::Beta::Avatar?
I realise the disadvantage of the above is that we would also have to reimplement or extend AvatarStack to make this work - since slots are fussy about their type
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.
General feedback - Part 2
I’m not keen on rendering in Rails and then adjusting the colour in JavaScript. That gives us two parallel codepaths (Ruby and TS) doing essentially the same work, with subtle colour differences due to hashing/overflow behaviour. It adds avoidable complexity and may introduce FOUC.
I think we should pick one direction:
- Unify everything in Ruby by resolving the hashing/colour-generation differences (if that's possible); or
- Make the server-side output minimal, e.g., render only a correctly sized placeholder circle to prevent reflow, and leave initials/colour entirely to the client.
If we keep server-side rendering, generate_fallback_svg and related methods should be extracted—either into a FallbackAvatarComponent or a small module with module_functions.
If we go with 2, then this begs the question as to whether we shouldn't just reuse the Angular implementation. If we do then this component probably belongs back in core for now.
Make `avatar-fallback` invisible to the layout system, removing them from the document flow. This allows the child .avatar img(s) to behave as if they are direct children of `.Avatar-body`
|
Thanks once more @myabc , those issues were definitely missed! Please review these changes that address the following:
op-avatar-demo.mp4 |
Primer::Beta::Avatar to render initials in a styled SVG when avatar img src is nil/blankPrimer::Beta::Avatar as Primer::OpenProject::AvatarWithFallback to render initials in a styled SVG when avatar img src is nil/blank
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
Copilot reviewed 21 out of 21 changed files in this pull request and generated 7 comments.
a4ddaed to
c692e44
Compare
Co-authored-by: Alexander Brandon Coles <[email protected]>
8e8817d to
9a5608c
Compare
What are you trying to accomplish?
Enhance
Primer::Beta::Avatarto render initials in a styled SVG when the avatar img src is nil/blank.This addresses OP Core Bug https://community.openproject.org/wp/69230 whereby avatars rendered via
Primer::Beta::AvatarStackdo not render with appropriately styled initials.Screenshots
Risk Assessment
What approach did you choose and why?
Use SVG data URIs with initials as fallback, keeping it as a proper
<img>element maintaining semantic HTML and ensuring screen readers handle avatars consistently.Anything you want to highlight for special attention from reviewers?
Warning
This change alters the
Primer::Beta::Avatarconstructor signature- needs review on whether that is acceptable for upstream (primer/view_components) syncing.Accessibility
Merge checklist