Merge branch '225920-replace-fa-bookmark-icons-with-gitlab-svg-bookmark-icon' into 'master'

Replace some FA icons on groups listing page

See merge request gitlab-org/gitlab!37162

app/helpers/icons_helper.rb

@@ -6,6 +6,8 @@ module IconsHelper
   extend self
   include FontAwesome::Rails::IconHelper
 
+  DEFAULT_ICON_SIZE = 16
+
   # Creates an icon tag given icon name(s) and possible icon modifiers.
   #
   # Right now this method simply delegates directly to `fa_icon` from the
@@ -21,7 +23,7 @@ module IconsHelper
     options.include?(:base) ? fa_stacked_icon(names, options) : fa_icon(names, options)
   end
 
-  def custom_icon(icon_name, size: 16)
+  def custom_icon(icon_name, size: DEFAULT_ICON_SIZE)
     # We can't simply do the below, because there are some .erb SVGs.
     #  File.read(Rails.root.join("app/views/shared/icons/_#{icon_name}.svg")).html_safe
     render "shared/icons/#{icon_name}.svg", size: size
@@ -52,7 +54,13 @@ module IconsHelper
     css_classes = []
     css_classes << "s#{size}" if size
     css_classes << "#{css_class}" unless css_class.blank?
-    content_tag(:svg, content_tag(:use, "", { "xlink:href" => "#{sprite_icon_path}##{icon_name}" } ), class: css_classes.empty? ? nil : css_classes.join(' '))
+
+    content_tag(
+      :svg,
+      content_tag(:use, '', { 'xlink:href' => "#{sprite_icon_path}##{icon_name}" } ),
+      class: css_classes.empty? ? nil : css_classes.join(' '),
+      data: { testid: "#{icon_name}-icon" }
+    )
   end
 
   def loading_icon(container: false, color: 'orange', size: 'sm', css_class: nil)
@@ -98,7 +106,7 @@ module IconsHelper
     end
   end
 
-  def visibility_level_icon(level, fw: true, options: {})
+  def visibility_level_icon(level, options: {})
     name =
       case level
       when Gitlab::VisibilityLevel::PRIVATE
@@ -106,13 +114,10 @@ module IconsHelper
       when Gitlab::VisibilityLevel::INTERNAL
         'shield'
       else # Gitlab::VisibilityLevel::PUBLIC
-        'globe'
+        'earth'
       end
 
-    name = [name]
-    name << "fw" if fw
-
-    icon(name.join(' '), options)
+    sprite_icon(name, size: DEFAULT_ICON_SIZE, css_class: 'gl-vertical-align-text-bottom')
   end
 
   def file_type_icon_class(type, mode, name)

app/views/admin/groups/_group.html.haml

@@ -22,15 +22,15 @@
     = render_if_exists 'admin/groups/marked_for_deletion_badge', group: group, css_class: 'gl-ml-5'
 
     %span.gl-ml-5
-      = icon('bookmark')
+      = sprite_icon('bookmark', size: 16, css_class: 'gl-vertical-align-text-bottom')
       = number_with_delimiter(group.projects.count)
 
     %span.gl-ml-5
-      = icon('users')
+      = sprite_icon('users', size: 16, css_class: 'gl-vertical-align-text-bottom')
       = number_with_delimiter(group.users.count)
 
     %span.gl-ml-5.visibility-icon.has-tooltip{ data: { container: 'body', placement: 'left' }, title: visibility_icon_description(group) }
-      = visibility_level_icon(group.visibility_level, fw: false)
+      = visibility_level_icon(group.visibility_level)
 
   .controls.gl-flex-shrink-0.gl-ml-5
     = link_to _('Edit'), admin_group_edit_path(group), id: "edit_#{dom_id(group)}", class: 'btn'

app/views/groups/_home_panel.html.haml

@@ -12,7 +12,7 @@
           %h1.home-panel-title.gl-mt-3.gl-mb-2
             = @group.name
             %span.visibility-icon.text-secondary.gl-ml-2.has-tooltip{ data: { container: 'body' }, title: visibility_icon_description(@group) }
-              = visibility_level_icon(@group.visibility_level, fw: false, options: {class: 'icon'})
+              = visibility_level_icon(@group.visibility_level, options: {class: 'icon'})
         .home-panel-metadata.d-flex.align-items-center.text-secondary
           %span
             = _("Group ID: %{group_id}") % { group_id: @group.id }

app/views/projects/_home_panel.html.haml

@@ -13,7 +13,7 @@
           %h1.home-panel-title.gl-mt-3.gl-mb-2{ data: { qa_selector: 'project_name_content' } }
             = @project.name
             %span.visibility-icon.text-secondary.gl-ml-2.has-tooltip{ data: { container: 'body' }, title: visibility_icon_description(@project) }
-              = visibility_level_icon(@project.visibility_level, fw: false, options: {class: 'icon'})
+              = visibility_level_icon(@project.visibility_level, options: {class: 'icon'})
             = render_if_exists 'compliance_management/compliance_framework/compliance_framework_badge', project: @project
         .home-panel-metadata.d-flex.flex-wrap.text-secondary
           - if can?(current_user, :read_project, @project)

app/views/shared/groups/_group.html.haml

@@ -18,12 +18,12 @@
 
   .stats.gl-text-gray-700.gl-flex-shrink-0
     %span.gl-ml-5
-      = icon('bookmark')
+      = sprite_icon('bookmark', size: 16, css_class: 'gl-vertical-align-text-bottom')
       = number_with_delimiter(group.projects.non_archived.count)
 
     %span.gl-ml-5
-      = icon('users')
+      = sprite_icon('users', size: 16, css_class: 'gl-vertical-align-text-bottom')
       = number_with_delimiter(group.users.count)
 
     %span.gl-ml-5.visibility-icon.has-tooltip{ data: { container: 'body', placement: 'left' }, title: visibility_icon_description(group) }
-      = visibility_level_icon(group.visibility_level, fw: false)
+      = visibility_level_icon(group.visibility_level)

app/views/shared/projects/_project.html.haml

@@ -40,7 +40,7 @@
                   = project.name
 
           %span.metadata-info.visibility-icon.gl-mr-3.gl-mt-3.text-secondary.has-tooltip{ data: { container: 'body', placement: 'top' }, title: visibility_icon_description(project) }
-            = visibility_level_icon(project.visibility_level, fw: true)
+            = visibility_level_icon(project.visibility_level)
 
           - if explore_projects_tab? && project_license_name(project)
             %span.metadata-info.d-inline-flex.align-items-center.gl-mr-3.gl-mt-3

app/views/shared/snippets/_header.html.haml

@@ -3,7 +3,7 @@
     .snippet-box.has-tooltip.inline.gl-mr-2{ title: snippet_visibility_level_description(@snippet.visibility_level, @snippet), data: { container: "body" } }
       %span.sr-only
         = visibility_level_label(@snippet.visibility_level)
-      = visibility_level_icon(@snippet.visibility_level, fw: false)
+      = visibility_level_icon(@snippet.visibility_level)
     %span.creator
       Authored
       = time_ago_with_tooltip(@snippet.created_at, placement: 'bottom', html_class: 'snippet_updated_ago')

app/views/shared/snippets/_snippet.html.haml

@@ -16,7 +16,7 @@
       %li
         %span.sr-only
           = visibility_level_label(snippet.visibility_level)
-        = visibility_level_icon(snippet.visibility_level, fw: false)
+        = visibility_level_icon(snippet.visibility_level)
 
   .snippet-info
     #{snippet.to_reference} ·

changelogs/unreleased/225920-replace-fa-bookmark-icons-with-gitlab-svg-bookmark-icon.yml

+---
+title: Replace some FA icons on groups listing page with GitLab SVG icons
+merge_request: 37162
+author:
+type: changed

spec/features/groups_spec.rb

@@ -34,7 +34,7 @@ RSpec.describe 'Group' do
 
         expect(group.visibility_level).to eq(Gitlab::VisibilityLevel::PUBLIC)
         expect(current_path).to eq(group_path(group))
-        expect(page).to have_selector '.visibility-icon .fa-globe'
+        expect(page).to have_selector '.visibility-icon [data-testid="earth-icon"]'
       end
     end
 

spec/features/snippets/user_edits_snippet_spec.rb

@@ -56,8 +56,8 @@ RSpec.describe 'User edits snippet', :js do
     click_button 'Save changes'
     wait_for_requests
 
-    expect(page).to have_no_xpath("//i[@class='fa fa-lock']")
-    expect(page).to have_xpath("//i[@class='fa fa-shield']")
+    expect(page).to have_no_selector('[data-testid="lock-icon"]')
+    expect(page).to have_selector('[data-testid="shield-icon"]')
   end
 
   it 'updates the snippet to make it public' do
@@ -66,8 +66,8 @@ RSpec.describe 'User edits snippet', :js do
     click_button 'Save changes'
     wait_for_requests
 
-    expect(page).to have_no_xpath("//i[@class='fa fa-lock']")
-    expect(page).to have_xpath("//i[@class='fa fa-globe']")
+    expect(page).to have_no_selector('[data-testid="lock-icon"]')
+    expect(page).to have_selector('[data-testid="earth-icon"]')
   end
 
   context 'when the git operation fails' do

spec/helpers/ci/status_helper_spec.rb

@@ -127,7 +127,7 @@ RSpec.describe Ci::StatusHelper do
       subject { helper.render_status_with_link("success", icon_size: 24) }
 
       it "has the svg class to change size" do
-        is_expected.to include("<svg class=\"s24\">")
+        is_expected.to include("<svg class=\"s24\"")
       end
     end
   end

spec/helpers/icons_helper_spec.rb

@@ -50,17 +50,17 @@ RSpec.describe IconsHelper do
 
     it 'returns svg icon html' do
       expect(sprite_icon(icon_name).to_s)
-        .to eq "<svg><use xlink:href=\"#{icons_path}##{icon_name}\"></use></svg>"
+        .to eq "<svg data-testid=\"#{icon_name}-icon\"><use xlink:href=\"#{icons_path}##{icon_name}\"></use></svg>"
     end
 
     it 'returns svg icon html + size classes' do
       expect(sprite_icon(icon_name, size: 72).to_s)
-        .to eq "<svg class=\"s72\"><use xlink:href=\"#{icons_path}##{icon_name}\"></use></svg>"
+        .to eq "<svg class=\"s72\" data-testid=\"#{icon_name}-icon\"><use xlink:href=\"#{icons_path}##{icon_name}\"></use></svg>"
     end
 
     it 'returns svg icon html + size classes + additional class' do
       expect(sprite_icon(icon_name, size: 72, css_class: 'icon-danger').to_s)
-        .to eq "<svg class=\"s72 icon-danger\"><use xlink:href=\"#{icons_path}##{icon_name}\"></use></svg>"
+        .to eq "<svg class=\"s72 icon-danger\" data-testid=\"#{icon_name}-icon\"><use xlink:href=\"#{icons_path}##{icon_name}\"></use></svg>"
     end
 
     describe 'non existing icon' do