Simplify inheritance structure to reuse super class constructor more effectively

Author: akabiru

app/components/primer/open_project/avatar_fallback.ts

@@ -1,4 +1,4 @@
-import {attr, controller} from '@github/catalyst'
+import { attr, controller } from '@github/catalyst'
 
 @controller
 export class AvatarFallbackElement extends HTMLElement {
@@ -8,19 +8,15 @@ export class AvatarFallbackElement extends HTMLElement {
   connectedCallback() {
     if (!this.uniqueId || !this.altText) return
 
-    const fallbackSvg = this.querySelector('svg[role="img"]')
-    if (!fallbackSvg) return
+    const img = this.querySelector('img[src^="data:image/svg+xml"]')
+    if (!img) return
 
     // Generate consistent color based on uniqueId and altText (hash must match OP Core)
     const text = `${this.uniqueId}${this.altText}`
     const hue = this.valueHash(text)
     const color = `hsl(${hue}, 50%, 30%)`
 
-    // Set background color on rect element
-    const rectElement = fallbackSvg.querySelector('rect')
-    if (rectElement) {
-      rectElement.setAttribute('fill', color)
-    }
+    this.updateSvgColor(img as HTMLImageElement, color)
   }
 
   /*
@@ -34,4 +30,14 @@ export class AvatarFallbackElement extends HTMLElement {
     }
     return hash % 360
   }
+
+  private updateSvgColor(img: HTMLImageElement, color: string) {
+    const dataUri = img.src
+    const base64 = dataUri.replace('data:image/svg+xml;base64,', '')
+    const svg = atob(base64)
+
+    const updatedSvg = svg.replace(/fill="hsl\([^"]+\)"/, `fill="${color}"`)
+
+    img.src = `data:image/svg+xml;base64,${btoa(updatedSvg)}`
+  }
 }

app/components/primer/open_project/avatar_stack.rb

@@ -13,18 +13,8 @@ class AvatarStack < Primer::Beta::AvatarStack
       renders_many :avatar_with_fallbacks, "Primer::OpenProject::AvatarWithFallback"
 
       # Alias avatar_with_fallbacks as avatars for use in the template
-      alias_method :avatars, :avatar_with_fallbacks
-
-      def before_render
-        @system_arguments[:classes] = class_names(
-          @system_arguments[:classes],
-          "AvatarStack--two" => avatar_with_fallbacks.size == 2,
-          "AvatarStack--three-plus" => avatar_with_fallbacks.size > 2
-        )
-      end
-
-      def render?
-        avatar_with_fallbacks.any?
+      def avatars
+        avatar_with_fallbacks
       end
     end
   end

app/components/primer/open_project/avatar_with_fallback.rb

@@ -27,71 +27,38 @@ class AvatarWithFallback < Primer::Beta::Avatar
       # @param unique_id [String, Integer] Unique identifier for generating consistent avatar colors across renders.
       # @param system_arguments [Hash] <%= link_to_system_arguments_docs %>
       def initialize(src: nil, alt: nil, size: DEFAULT_SIZE, shape: DEFAULT_SHAPE, href: nil, unique_id: nil, **system_arguments)
-        @unique_id = unique_id
-        @src = src
+        require_src_or_alt_arguments(src, alt)
 
-        if src.present?
-          super(src: src, alt: alt, size: size, shape: shape, href: href, **system_arguments)
-        else
-          @alt = alt
-          @href = href
-          @size = fetch_or_fallback(SIZE_OPTIONS, size, DEFAULT_SIZE)
-          @shape = fetch_or_fallback(SHAPE_OPTIONS, shape, DEFAULT_SHAPE)
-          @system_arguments = deny_tag_argument(**system_arguments)
+        @unique_id = unique_id
+        @use_fallback = src.blank?
+        final_src = @use_fallback ? generate_fallback_svg(alt, size) : src
 
-          validate_src_or_alt(nil, alt)
-        end
+        super(src: final_src, alt: alt, size: size, shape: shape, href: href, **system_arguments)
       end
 
       def call
-        if @src.present?
-          super
+        if @use_fallback
+          render(Primer::BaseComponent.new(
+            tag: :"avatar-fallback",
+            data: {
+              unique_id: @unique_id,
+              alt_text: @system_arguments[:alt]
+            }
+          )) { super }
         else
-          render_fallback
+          super
         end
       end
 
       private
 
-      def validate_src_or_alt(src, alt)
+      def require_src_or_alt_arguments(src, alt)
         return if src.present? || alt.present?
 
         raise ArgumentError, "`src` or `alt` is required"
       end
 
-      def fallback_font_size
-        # Calculate font size for initials (45% of avatar size, min 8px)
-        [(@size * 0.45).round, 8].max
-      end
-
-      def fallback_avatar_classes
-        class_names(
-          @system_arguments[:classes],
-          "avatar",
-          "avatar-#{@size}",
-          "avatar-small" => @size < SMALL_THRESHOLD,
-          "circle" => @shape == DEFAULT_SHAPE
-        )
-      end
-
-      def extract_initials(name)
-        return "" if name.blank?
-
-        chars = name.chars
-        first = chars[0]&.upcase || ""
-
-        last_space = name.rindex(" ")
-        if last_space && last_space < name.length - 1
-          last = name[last_space + 1]&.upcase || ""
-          "#{first}#{last}"
-        else
-          first
-        end
-      end
-
-      def render_fallback
-        initials = extract_initials(@alt)
-
+      def generate_fallback_svg(alt, size)
         svg_content = content_tag(
           :svg,
           safe_join([
@@ -100,40 +67,46 @@ def render_fallback
             tag.rect(width: "100%", height: "100%", fill: "hsl(0, 0%, 35%)"),
             content_tag(
               :text,
-              initials,
+              extract_initials(alt),
               x: "50%",
               y: "50%",
               "text-anchor": "middle",
               "dominant-baseline": "central",
               fill: "white",
-              "font-size": fallback_font_size,
+              "font-size": fallback_font_size(size),
               "font-weight": "600",
               "font-family": FONT_STACK,
               style: "user-select: none; text-transform: uppercase;"
             )
           ]),
-          width: @size,
-          height: @size,
-          viewBox: "0 0 #{@size} #{@size}",
-          role: "img",
-          "aria-label": @alt,
-          class: fallback_avatar_classes
+          xmlns: "http://www.w3.org/2000/svg",
+          width: size,
+          height: size,
+          viewBox: "0 0 #{size} #{size}",
         )
 
-        render(Primer::BaseComponent.new(
-          tag: :"avatar-fallback",
-          data: {
-            unique_id: @unique_id,
-            alt_text: @alt
-          }
-        )) do
-          render(Primer::ConditionalWrapper.new(
-            condition: @href.present?,
-            component: Primer::Beta::Link,
-            **(@href.present? ? { href: @href } : {})
-          )) { svg_content }
+        "data:image/svg+xml;base64,#{Base64.strict_encode64(svg_content)}"
+      end
+
+      def extract_initials(name)
+        return "" if name.blank?
+
+        chars = name.chars
+        first = chars[0]&.upcase || ""
+
+        last_space = name.rindex(" ")
+        if last_space && last_space < name.length - 1
+          last = name[last_space + 1]&.upcase || ""
+          "#{first}#{last}"
+        else
+          first
         end
       end
+
+      def fallback_font_size(size)
+        # Font size is 45% of avatar size for good readability, with a minimum of 8px
+        [(size * 0.45).round, 8].max
+      end
     end
   end
 end

previews/primer/open_project/avatar_with_fallback_preview.rb

@@ -2,7 +2,7 @@
 
 module Primer
   module OpenProject
-    # @label OpenProject Avatar
+    # @label OpenProject AvatarWithFallback
     class AvatarWithFallbackPreview < ViewComponent::Preview
       # @label Playground
       #

test/components/open_project/avatar_stack_test.rb

@@ -26,7 +26,7 @@ def test_renders_with_fallback_avatars
     assert_selector("div.AvatarStack") do
       assert_selector(".AvatarStack-body") do
         assert_selector("avatar-fallback", count: 2)
-        assert_selector("svg.avatar[role='img']", count: 2)
+        assert_selector("img.avatar[src^='data:image/svg+xml;base64,']", count: 2)
       end
     end
   end
@@ -39,9 +39,10 @@ def test_renders_mixed_avatars
 
     assert_selector("div.AvatarStack") do
       assert_selector(".AvatarStack-body") do
-        assert_selector("img.avatar", count: 1)
+        # 2 img tags total: 1 with remote src, 1 with data URI fallback
+        assert_selector("img.avatar", count: 2)
         assert_selector("avatar-fallback", count: 1)
-        assert_selector("svg.avatar[role='img']", count: 1)
+        assert_selector("img.avatar[src^='data:image/svg+xml;base64,']", count: 1)
       end
     end
   end
@@ -56,7 +57,7 @@ def test_renders_three_plus_fallback_avatars
     assert_selector(".AvatarStack.AvatarStack--three-plus") do
       assert_selector(".AvatarStack-body") do
         assert_selector("avatar-fallback", count: 3)
-        assert_selector("svg.avatar[role='img']", count: 3)
+        assert_selector("img.avatar[src^='data:image/svg+xml;base64,']", count: 3)
       end
     end
   end

test/components/open_project/avatar_with_fallback_test.rb

@@ -15,20 +15,18 @@ def test_renders_image_avatar_with_src
   def test_renders_fallback_when_src_is_nil
     render_inline(Primer::OpenProject::AvatarWithFallback.new(src: nil, alt: "OpenProject Admin"))
 
-    # Should render avatar-fallback element wrapping a svg[role="img"]
+    # Should render avatar-fallback element wrapping an img with base64 SVG data URI
     assert_selector("avatar-fallback[data-alt-text='OpenProject Admin']") do
-      assert_selector("svg.avatar[role='img'][aria-label='OpenProject Admin']")
+      assert_selector("img.avatar[src^='data:image/svg+xml;base64,']")
     end
-    refute_selector("img")
   end
 
   def test_renders_fallback_when_src_is_blank
     render_inline(Primer::OpenProject::AvatarWithFallback.new(src: "", alt: "OpenProject Admin"))
 
     assert_selector("avatar-fallback[data-alt-text='OpenProject Admin']") do
-      assert_selector("svg.avatar[role='img']")
+      assert_selector("img.avatar[src^='data:image/svg+xml;base64,']")
     end
-    refute_selector("img")
   end
 
   def test_defaults_to_size_20
@@ -40,7 +38,7 @@ def test_defaults_to_size_20
   def test_fallback_defaults_to_size_20
     render_inline(Primer::OpenProject::AvatarWithFallback.new(src: nil, alt: "Test User"))
 
-    assert_selector("svg.avatar.avatar-20")
+    assert_selector("img.avatar[src^='data:image/svg+xml;base64,']")
   end
 
   def test_falls_back_when_size_isn_t_valid
@@ -60,7 +58,7 @@ def test_defaults_to_circle_avatar
   def test_fallback_defaults_to_circle_avatar
     render_inline(Primer::OpenProject::AvatarWithFallback.new(src: nil, alt: "Test User"))
 
-    assert_selector("svg.avatar.circle")
+    assert_selector("img.avatar.circle[src^='data:image/svg+xml;base64,']")
   end
 
   def test_adds_small_modifier_when_size_is_less_than_threshold
@@ -72,7 +70,7 @@ def test_adds_small_modifier_when_size_is_less_than_threshold
   def test_fallback_adds_small_modifier_when_size_is_less_than_threshold
     render_inline(Primer::OpenProject::AvatarWithFallback.new(src: nil, alt: "Test User", size: Primer::OpenProject::AvatarWithFallback::SMALL_THRESHOLD - 4))
 
-    assert_selector("svg.avatar.avatar-small")
+    assert_selector("img.avatar.avatar-small[src^='data:image/svg+xml;base64,']")
   end
 
   def test_sets_size_height_and_width
@@ -84,7 +82,8 @@ def test_sets_size_height_and_width
   def test_fallback_sets_correct_size_class
     render_inline(Primer::OpenProject::AvatarWithFallback.new(src: nil, alt: "Test User", size: 40))
 
-    assert_selector("svg.avatar.avatar-40")
+    # Size is set via attributes, not a dedicated class
+    assert_selector("img.avatar[size='40'][height='40'][width='40'][src^='data:image/svg+xml;base64,']")
   end
 
   def test_squared_avatar
@@ -97,7 +96,7 @@ def test_squared_avatar
   def test_fallback_squared_avatar
     render_inline(Primer::OpenProject::AvatarWithFallback.new(src: nil, alt: "Test User", shape: :square))
 
-    assert_selector("svg.avatar")
+    assert_selector("img.avatar[src^='data:image/svg+xml;base64,']")
     refute_selector(".circle")
   end
 
@@ -114,9 +113,10 @@ def test_renders_link_wrapper
   def test_fallback_renders_link_wrapper
     render_inline(Primer::OpenProject::AvatarWithFallback.new(src: nil, alt: "Test User", href: "#test"))
 
+    # When href is provided, the avatar class is on the <a> tag, not the <img>
     assert_selector("avatar-fallback") do
-      assert_selector("a[href='#test']") do
-        assert_selector("svg.avatar[role='img']")
+      assert_selector("a.avatar[href='#test']") do
+        assert_selector("img[src^='data:image/svg+xml;base64,']")
       end
     end
   end
@@ -126,7 +126,7 @@ def test_fallback_with_unique_id_in_data_attribute
 
     # Should have data attributes for client-side processing
     assert_selector("avatar-fallback[data-unique-id='123'][data-alt-text='Test User']") do
-      assert_selector("svg.avatar[role='img']")
+      assert_selector("img.avatar[src^='data:image/svg+xml;base64,']")
     end
   end
 
@@ -135,7 +135,7 @@ def test_fallback_without_unique_id
 
     # Should still render, just without unique_id data attribute
     assert_selector("avatar-fallback[data-alt-text='Test User']") do
-      assert_selector("svg.avatar[role='img']")
+      assert_selector("img.avatar[src^='data:image/svg+xml;base64,']")
     end
   end
 
@@ -148,7 +148,7 @@ def test_adds_custom_classes
   def test_fallback_adds_custom_classes
     render_inline(Primer::OpenProject::AvatarWithFallback.new(src: nil, alt: "Test User", classes: "custom-class"))
 
-    assert_selector("svg.avatar.custom-class")
+    assert_selector("img.avatar.custom-class[src^='data:image/svg+xml;base64,']")
   end
 
   def test_raises_when_both_src_and_alt_are_missing