From 1a71b1721f1369664a9e328c8b39bb1ef2f87a80 Mon Sep 17 00:00:00 2001
From: Mark Florian <mflorian@gitlab.com>
Date: Tue, 1 Mar 2022 10:17:27 +0000
Subject: [PATCH] Make loading_icon helper conform to GlLoadingIcon

This renames the `loading_icon` helper to `gl_loading_icon`, and makes
its API and output more similar to the `GlLoadingIcon` Vue component:

- The container element is now always rendered.
- The container element is a block-level element by default.
- The `container` argument has been replaced with with `inline`,
  defaulting to `false`.
- The container element now has `role="status"` for better
  accessibility.
- The default colour of the spinner is `dark`, as `orange` is not
  a valid colour (see
  https://gitlab.com/gitlab-org/gitlab-ui/-/issues/1029#note_431080670).

Part of https://gitlab.com/gitlab-org/gitlab/-/issues/352510, to aid the
migration described in
https://gitlab.com/groups/gitlab-org/-/epics/7628.
---
 app/helpers/dropdowns_helper.rb               |  2 +-
 app/helpers/icons_helper.rb                   | 40 +++++++++++++++----
 app/views/projects/_files.html.haml           |  3 +-
 app/views/projects/blob/_blob.html.haml       |  3 +-
 .../viewers/_gitlab_ci_yml_loading.html.haml  |  2 +-
 .../projects/blob/viewers/_loading.html.haml  |  3 +-
 .../blob/viewers/_loading_auxiliary.html.haml |  2 +-
 .../_metrics_dashboard_yml_loading.html.haml  |  2 +-
 .../blob/viewers/_route_map_loading.html.haml |  2 +-
 .../projects/blob/viewers/_sketch.html.haml   |  3 +-
 .../projects/blob/viewers/_stl.html.haml      |  2 +-
 app/views/projects/commits/show.html.haml     |  3 +-
 app/views/projects/find_file/show.html.haml   |  3 +-
 app/views/projects/imports/show.html.haml     |  2 +-
 .../prometheus/_custom_metrics.html.haml      |  2 +-
 .../services/prometheus/_metrics.html.haml    |  2 +-
 .../shared/_new_project_item_select.html.haml |  2 +-
 app/views/shared/issuable/_sidebar.html.haml  |  4 +-
 .../issuable/_sidebar_assignees.html.haml     |  2 +-
 .../issuable/_sidebar_reviewers.html.haml     |  2 +-
 app/views/shared/notes/_hints.html.haml       |  2 +-
 doc/development/fe_guide/icons.md             | 34 +++++-----------
 spec/helpers/icons_helper_spec.rb             | 32 ++++++++++-----
 23 files changed, 85 insertions(+), 69 deletions(-)

diff --git a/app/helpers/dropdowns_helper.rb b/app/helpers/dropdowns_helper.rb
index 0092743f96ef1..a910d3d7c9d4d 100644
--- a/app/helpers/dropdowns_helper.rb
+++ b/app/helpers/dropdowns_helper.rb
@@ -129,7 +129,7 @@ def dropdown_footer(add_content_class: false, &block)
   end
 
   def dropdown_loading
-    spinner = loading_icon(container: true, size: "md", css_class: "gl-mt-7")
+    spinner = gl_loading_icon(size: "md", css_class: "gl-mt-7")
     content_tag(:div, spinner, class: "dropdown-loading")
   end
 end
diff --git a/app/helpers/icons_helper.rb b/app/helpers/icons_helper.rb
index 32d808c960c0b..6f7ac069fe487 100644
--- a/app/helpers/icons_helper.rb
+++ b/app/helpers/icons_helper.rb
@@ -49,13 +49,39 @@ def sprite_icon(icon_name, size: DEFAULT_ICON_SIZE, css_class: nil)
     end
   end
 
-  def loading_icon(container: false, color: 'orange', size: 'sm', css_class: nil)
-    css_classes = ['gl-spinner', "gl-spinner-#{color}", "gl-spinner-#{size}"]
-    css_classes << "#{css_class}" unless css_class.blank?
-
-    spinner = content_tag(:span, "", { class: css_classes.join(' '), aria: { label: _('Loading') } })
-
-    container == true ? content_tag(:div, spinner, { class: 'gl-spinner-container' }) : spinner
+  # Creates a GitLab UI loading icon/spinner.
+  #
+  # Examples:
+  #   # Default
+  #   gl_loading_icon
+  #
+  #   # Sizes
+  #   gl_loading_icon(size: 'md')
+  #   gl_loading_icon(size: 'lg')
+  #   gl_loading_icon(size: 'xl')
+  #
+  #   # Colors
+  #   gl_loading_icon(color: 'light')
+  #
+  #   # Block/Inline
+  #   gl_loading_icon(inline: true)
+  #
+  #   # Custom classes
+  #   gl_loading_icon(css_class: "foo-bar")
+  #
+  # See also https://gitlab-org.gitlab.io/gitlab-ui/?path=/story/base-loading-icon--default
+  def gl_loading_icon(inline: false, color: 'dark', size: 'sm', css_class: nil)
+    spinner = content_tag(:span, "", {
+      class: %[gl-spinner gl-spinner-#{color} gl-spinner-#{size} gl-vertical-align-text-bottom!],
+      aria: { label: _('Loading') }
+    })
+
+    container_classes = ['gl-spinner-container']
+    container_classes << css_class unless css_class.blank?
+    content_tag(inline ? :span : :div, spinner, {
+      class: container_classes,
+      role: 'status'
+    })
   end
 
   def external_snippet_icon(name)
diff --git a/app/views/projects/_files.html.haml b/app/views/projects/_files.html.haml
index 2f4a61865f85e..a7cf50623f0c9 100644
--- a/app/views/projects/_files.html.haml
+++ b/app/views/projects/_files.html.haml
@@ -12,8 +12,7 @@
 
   .info-well.gl-display-none.gl-sm-display-flex.project-last-commit.gl-flex-direction-column
     #js-last-commit.gl-m-auto
-      .gl-spinner-container.m-auto
-        = loading_icon(size: 'md', color: 'dark', css_class: 'align-text-bottom')
+      = gl_loading_icon(size: 'md')
     #js-code-owners
 
   - if is_project_overview
diff --git a/app/views/projects/blob/_blob.html.haml b/app/views/projects/blob/_blob.html.haml
index 919cafe7ce8ae..85b9a69ab4cc6 100644
--- a/app/views/projects/blob/_blob.html.haml
+++ b/app/views/projects/blob/_blob.html.haml
@@ -21,8 +21,7 @@
                               project_path: @project.full_path,
                               target_branch: project.empty_repo? ? ref : @ref,
                               original_branch: @ref } }
-      .gl-spinner-container
-        = loading_icon(size: 'md')
+      = gl_loading_icon(size: 'md')
   - else
     %article.file-holder
       = render 'projects/blob/header', blob: blob
diff --git a/app/views/projects/blob/viewers/_gitlab_ci_yml_loading.html.haml b/app/views/projects/blob/viewers/_gitlab_ci_yml_loading.html.haml
index cf57f1b531d7e..2b8f62d98bf88 100644
--- a/app/views/projects/blob/viewers/_gitlab_ci_yml_loading.html.haml
+++ b/app/views/projects/blob/viewers/_gitlab_ci_yml_loading.html.haml
@@ -1,4 +1,4 @@
-= loading_icon(css_class: "gl-vertical-align-text-bottom mr-1")
+= gl_loading_icon(inline: true, css_class: "gl-mr-2!")
 = s_('Pipelines|Validating GitLab CI configuration…')
 
 = link_to _('Learn more'), help_page_path('ci/yaml/index')
diff --git a/app/views/projects/blob/viewers/_loading.html.haml b/app/views/projects/blob/viewers/_loading.html.haml
index 18fd0d87ce63d..9cb934da7c0a0 100644
--- a/app/views/projects/blob/viewers/_loading.html.haml
+++ b/app/views/projects/blob/viewers/_loading.html.haml
@@ -1,2 +1 @@
-.text-center.gl-mt-4.gl-mb-3
-  = loading_icon(size: "md", css_class: "qa-spinner")
+= gl_loading_icon(size: "md", css_class: "qa-spinner gl-my-4")
diff --git a/app/views/projects/blob/viewers/_loading_auxiliary.html.haml b/app/views/projects/blob/viewers/_loading_auxiliary.html.haml
index 5a2212e0b4eb5..19aa96a930220 100644
--- a/app/views/projects/blob/viewers/_loading_auxiliary.html.haml
+++ b/app/views/projects/blob/viewers/_loading_auxiliary.html.haml
@@ -1,2 +1,2 @@
-= loading_icon(css_class: "gl-vertical-align-text-bottom")
+= gl_loading_icon(inline: true)
 = _("Analyzing file…")
diff --git a/app/views/projects/blob/viewers/_metrics_dashboard_yml_loading.html.haml b/app/views/projects/blob/viewers/_metrics_dashboard_yml_loading.html.haml
index db4b04eaeb880..5e355ecc4b8cc 100644
--- a/app/views/projects/blob/viewers/_metrics_dashboard_yml_loading.html.haml
+++ b/app/views/projects/blob/viewers/_metrics_dashboard_yml_loading.html.haml
@@ -1,4 +1,4 @@
-= loading_icon(css_class: "gl-vertical-align-text-bottom mr-1")
+= gl_loading_icon(inline: true, css_class: "mr-1")
 = _('Metrics Dashboard YAML definition') + '…'
 
 = link_to _('Learn more'), help_page_path('operations/metrics/dashboards/yaml.md')
diff --git a/app/views/projects/blob/viewers/_route_map_loading.html.haml b/app/views/projects/blob/viewers/_route_map_loading.html.haml
index c48ab84654f0c..d9e965246a828 100644
--- a/app/views/projects/blob/viewers/_route_map_loading.html.haml
+++ b/app/views/projects/blob/viewers/_route_map_loading.html.haml
@@ -1,4 +1,4 @@
-= loading_icon(css_class: "gl-vertical-align-text-bottom gl-mr-1")
+= gl_loading_icon(inline: true, css_class: "gl-mr-1")
 Validating Route Map…
 
 = link_to 'Learn more', help_page_path('ci/environments/index.md', anchor: 'go-from-source-files-to-public-pages')
diff --git a/app/views/projects/blob/viewers/_sketch.html.haml b/app/views/projects/blob/viewers/_sketch.html.haml
index 08c21258d3fc8..4feaa7392fd74 100644
--- a/app/views/projects/blob/viewers/_sketch.html.haml
+++ b/app/views/projects/blob/viewers/_sketch.html.haml
@@ -1,3 +1,2 @@
 .file-content#js-sketch-viewer{ data: { endpoint: blob_raw_path } }
-  .text-center.gl-mt-4.gl-mb-3.js-loading-icon
-    = loading_icon(size: "md")
+  = gl_loading_icon(size: "md", css_class: "gl-my-4 js-loading-icon")
diff --git a/app/views/projects/blob/viewers/_stl.html.haml b/app/views/projects/blob/viewers/_stl.html.haml
index f98deebacf9ff..8bf0339fc3c1e 100644
--- a/app/views/projects/blob/viewers/_stl.html.haml
+++ b/app/views/projects/blob/viewers/_stl.html.haml
@@ -1,6 +1,6 @@
 .file-content.is-stl-loading
   .text-center#js-stl-viewer{ data: { endpoint: blob_raw_path } }
-    = loading_icon(size: "md", css_class: "gl-mt-4 gl-mb-3")
+    = gl_loading_icon(size: "md", css_class: "gl-my-4")
   .text-center.gl-mt-3.gl-mb-3.stl-controls
     .btn-group
       %button.gl-button.btn.btn-default.btn-sm.js-material-changer{ data: { type: 'wireframe' } }
diff --git a/app/views/projects/commits/show.html.haml b/app/views/projects/commits/show.html.haml
index 22a5bada3117b..36641a8c508fd 100644
--- a/app/views/projects/commits/show.html.haml
+++ b/app/views/projects/commits/show.html.haml
@@ -34,5 +34,4 @@
   %div{ id: dom_id(@project) }
     %ol#commits-list.list-unstyled.content_list
       = render 'commits', project: @project, ref: @ref
-  .loading.hide
-    = loading_icon(size: "lg")
+  = gl_loading_icon(size: 'lg', css_class: 'loading hide')
diff --git a/app/views/projects/find_file/show.html.haml b/app/views/projects/find_file/show.html.haml
index 194b10e9ef4f7..af5ad06d30e8d 100644
--- a/app/views/projects/find_file/show.html.haml
+++ b/app/views/projects/find_file/show.html.haml
@@ -23,5 +23,4 @@
               = _('There are no matching files')
             %p.text-secondary
               = _('Try using a different search term to find the file you are looking for.')
-      .text-center.gl-mt-3.loading
-        = loading_icon(size: 'md')
+      = gl_loading_icon(size: 'md', css_class: 'gl-mt-3 loading')
diff --git a/app/views/projects/imports/show.html.haml b/app/views/projects/imports/show.html.haml
index 0c1efab21951f..8096bc6cead94 100644
--- a/app/views/projects/imports/show.html.haml
+++ b/app/views/projects/imports/show.html.haml
@@ -4,7 +4,7 @@
 .save-project-loader
   .center
     %h2
-      = loading_icon
+      = gl_loading_icon(inline: true)
       = import_in_progress_title
     - if !has_ci_cd_only_params? && @project.external_import?
       %p.monospace git clone --bare #{@project.safe_import_url}
diff --git a/app/views/projects/services/prometheus/_custom_metrics.html.haml b/app/views/projects/services/prometheus/_custom_metrics.html.haml
index 4586ee844c0b7..896249c6163ca 100644
--- a/app/views/projects/services/prometheus/_custom_metrics.html.haml
+++ b/app/views/projects/services/prometheus/_custom_metrics.html.haml
@@ -18,7 +18,7 @@
           .flash-text
       .loading-metrics.js-loading-custom-metrics
         %p.m-3
-          = loading_icon(css_class: 'metrics-load-spinner')
+          = gl_loading_icon(inline: true, css_class: 'metrics-load-spinner')
           = s_('PrometheusService|Finding custom metrics...')
       .empty-metrics.hidden.js-empty-custom-metrics
         %p.text-tertiary.m-3.js-no-active-integration-text.hidden
diff --git a/app/views/projects/services/prometheus/_metrics.html.haml b/app/views/projects/services/prometheus/_metrics.html.haml
index 0d41584652f95..8794f3e24dae2 100644
--- a/app/views/projects/services/prometheus/_metrics.html.haml
+++ b/app/views/projects/services/prometheus/_metrics.html.haml
@@ -16,7 +16,7 @@
     .card-body
       .loading-metrics.js-loading-metrics
         %p.m-3
-          = loading_icon(css_class: 'metrics-load-spinner')
+          = gl_loading_icon(inline: true, css_class: 'metrics-load-spinner')
           = s_('PrometheusService|Finding and configuring metrics...')
       .empty-metrics.hidden.js-empty-metrics
         %p.text-tertiary.m-3
diff --git a/app/views/shared/_new_project_item_select.html.haml b/app/views/shared/_new_project_item_select.html.haml
index 08003346d0904..74a397d7a03f4 100644
--- a/app/views/shared/_new_project_item_select.html.haml
+++ b/app/views/shared/_new_project_item_select.html.haml
@@ -1,7 +1,7 @@
 - if any_projects?(@projects)
   .project-item-select-holder.btn-group.gl-ml-auto.gl-mr-auto.gl-relative.gl-overflow-hidden{ class: 'gl-display-flex!' }
     %a.btn.gl-button.btn-confirm.js-new-project-item-link.block-truncated.qa-new-project-item-link{ href: '', data: { label: local_assigns[:label], type: local_assigns[:type] }, class: "gl-m-0!" }
-      = loading_icon(color: 'light')
+      = gl_loading_icon(inline: true, color: 'light')
     = project_select_tag :project_path, class: "project-item-select gl-absolute! gl-visibility-hidden", data: { include_groups: local_assigns[:include_groups], order_by: 'last_activity_at', relative_path: local_assigns[:path], with_shared: local_assigns[:with_shared], include_projects_in_subgroups: local_assigns[:include_projects_in_subgroups] }, with_feature_enabled: local_assigns[:with_feature_enabled]
     %button.btn.dropdown-toggle.btn-confirm.btn-md.gl-button.gl-dropdown-toggle.dropdown-toggle-split.new-project-item-select-button.qa-new-project-item-select-button.gl-p-0.gl-w-100{ class: "gl-m-0!", 'aria-label': _('Toggle project select') }
       = sprite_icon('chevron-down')
diff --git a/app/views/shared/issuable/_sidebar.html.haml b/app/views/shared/issuable/_sidebar.html.haml
index 7787e5dd66080..f966958d2c73c 100644
--- a/app/views/shared/issuable/_sidebar.html.haml
+++ b/app/views/shared/issuable/_sidebar.html.haml
@@ -50,7 +50,7 @@
           // Fallback while content is loading
           .title.hide-collapsed
             = _('Time tracking')
-            = loading_icon(css_class: 'gl-vertical-align-text-bottom')
+            = gl_loading_icon(inline: true)
       - if issuable_sidebar.has_key?(:due_date)
         #js-due-date-entry-point
 
@@ -109,8 +109,8 @@
               = dropdown_loading
               = dropdown_footer add_content_class: true do
                 %button.gl-button.btn.btn-confirm.sidebar-move-issue-confirmation-button.js-move-issue-confirmation-button{ type: 'button', disabled: true }
+                  = gl_loading_icon(inline: true, css_class: 'sidebar-move-issue-confirmation-loading-icon gl-mr-2')
                   = _('Move')
-                  = loading_icon(css_class: 'gl-vertical-align-text-bottom sidebar-move-issue-confirmation-loading-icon')
 
     -# haml-lint:disable InlineJavaScript
     %script.js-sidebar-options{ type: "application/json" }= issuable_sidebar_options(issuable_sidebar).to_json.html_safe
diff --git a/app/views/shared/issuable/_sidebar_assignees.html.haml b/app/views/shared/issuable/_sidebar_assignees.html.haml
index 9a0b25f401536..2fd4c598580c2 100644
--- a/app/views/shared/issuable/_sidebar_assignees.html.haml
+++ b/app/views/shared/issuable/_sidebar_assignees.html.haml
@@ -7,7 +7,7 @@
   directly_invite_members: can_admin_project_member?(@project) } }
   .title.hide-collapsed
     = _('Assignee')
-    = loading_icon(css_class: 'gl-vertical-align-text-bottom')
+    = gl_loading_icon(inline: true)
 
 .js-sidebar-assignee-data.selectbox.hide-collapsed
   - if assignees.none?
diff --git a/app/views/shared/issuable/_sidebar_reviewers.html.haml b/app/views/shared/issuable/_sidebar_reviewers.html.haml
index bc76d292dd64b..ce252e7457081 100644
--- a/app/views/shared/issuable/_sidebar_reviewers.html.haml
+++ b/app/views/shared/issuable/_sidebar_reviewers.html.haml
@@ -3,7 +3,7 @@
 #js-vue-sidebar-reviewers{ data: { field: issuable_type, signed_in: signed_in } }
   .title.hide-collapsed
     = _('Reviewer')
-    = loading_icon(css_class: 'gl-vertical-align-text-bottom')
+    = gl_loading_icon(inline: true)
 
 .selectbox.hide-collapsed
   - if reviewers.none?
diff --git a/app/views/shared/notes/_hints.html.haml b/app/views/shared/notes/_hints.html.haml
index 6c8b2a9e5bb39..8a79a17b16653 100644
--- a/app/views/shared/notes/_hints.html.haml
+++ b/app/views/shared/notes/_hints.html.haml
@@ -18,7 +18,7 @@
         %span.attaching-file-message
           -# Populated by app/assets/javascripts/dropzone_input.js
         %span.uploading-progress 0%
-        = loading_icon(css_class: 'align-text-bottom gl-mr-2')
+        = gl_loading_icon(inline: true, css_class: 'gl-mr-2')
 
       %span.uploading-error-container.hide
         %span.uploading-error-icon
diff --git a/doc/development/fe_guide/icons.md b/doc/development/fe_guide/icons.md
index 3f7490b0221aa..d107af156db63 100644
--- a/doc/development/fe_guide/icons.md
+++ b/doc/development/fe_guide/icons.md
@@ -88,50 +88,36 @@ Please use the following function inside JS to render an icon:
 
 ### Usage in HAML/Rails
 
-To insert a loading spinner in HAML or Rails use the `loading_icon` helper:
+To insert a loading spinner in HAML or Rails use the `gl_loading_icon` helper:
 
 ```haml
-= loading_icon
+= gl_loading_icon
 ```
 
-You can include one or more of the following properties with the `loading_icon` helper, as demonstrated
+You can include one or more of the following properties with the `gl_loading_icon` helper, as demonstrated
 by the examples that follow:
 
-- `container` (optional): wraps the loading icon in a container, which centers the loading icon using the `text-center` CSS property.
-- `color` (optional): either `orange` (default), `light`, or `dark`.
+- `inline` (optional): uses in an inline element if `true`, otherwise, a block element (default), with the spinner centered.
+- `color` (optional): either `dark` (default) or `light`.
 - `size` (optional): either `sm` (default), `md`, `lg`, or `xl`.
-- `css_class` (optional): defaults to an empty string, but can be used for utility classes to fine-tune alignment or spacing.
+- `css_class` (optional): defaults to nothing, but can be used for utility classes to fine-tune alignment or spacing.
 
 **Example 1:**
 
 The following HAML expression generates a loading icon's markup and
-centers the icon by wrapping it in a `gl-spinner-container` element.
+centers the icon.
 
 ```haml
-= loading_icon(container: true)
-```
-
-**Output from example 1:**
-
-```html
-<div class="gl-spinner-container">
-  <span class="gl-spinner gl-spinner-orange gl-spinner-sm" aria-label="Loading"></span>
-</div>
+= gl_loading_icon
 ```
 
 **Example 2:**
 
-The following HAML expression generates a loading icon's markup
+The following HAML expression generates an inline loading icon's markup
 with a custom size. It also appends a margin utility class.
 
 ```haml
-= loading_icon(size: 'lg', css_class: 'gl-mr-2')
-```
-
-**Output from example 2:**
-
-```html
-<span class="gl-spinner gl-spinner-orange gl-spinner-lg gl-mr-2" aria-label="Loading"></span>
+= gl_loading_icon(inline: true, size: 'lg', css_class: 'gl-mr-2')
 ```
 
 ### Usage in Vue
diff --git a/spec/helpers/icons_helper_spec.rb b/spec/helpers/icons_helper_spec.rb
index af2957d72c7bc..139e8be33d54a 100644
--- a/spec/helpers/icons_helper_spec.rb
+++ b/spec/helpers/icons_helper_spec.rb
@@ -231,23 +231,33 @@
     end
   end
 
-  describe 'loading_icon' do
-    it 'returns span with gl-spinner class and default configuration' do
-      expect(loading_icon.to_s)
-        .to eq '<span class="gl-spinner gl-spinner-orange gl-spinner-sm" aria-label="Loading"></span>'
+  describe 'gl_loading_icon' do
+    it 'returns the default spinner markup' do
+      expect(gl_loading_icon.to_s)
+        .to eq '<div class="gl-spinner-container" role="status"><span class="gl-spinner gl-spinner-dark gl-spinner-sm gl-vertical-align-text-bottom!" aria-label="Loading"></span></div>'
     end
 
     context 'when css_class is provided' do
-      it 'appends css_class to gl-spinner element' do
-        expect(loading_icon(css_class: 'gl-mr-2').to_s)
-          .to eq '<span class="gl-spinner gl-spinner-orange gl-spinner-sm gl-mr-2" aria-label="Loading"></span>'
+      it 'appends css_class to container element' do
+        expect(gl_loading_icon(css_class: 'gl-mr-2').to_s).to match 'gl-spinner-container gl-mr-2'
       end
     end
 
-    context 'when container is true' do
-      it 'creates a container that has the gl-spinner-container class selector' do
-        expect(loading_icon(container: true).to_s)
-        .to eq '<div class="gl-spinner-container"><span class="gl-spinner gl-spinner-orange gl-spinner-sm" aria-label="Loading"></span></div>'
+    context 'when size is provided' do
+      it 'sets the size class' do
+        expect(gl_loading_icon(size: 'xl').to_s).to match 'gl-spinner-xl'
+      end
+    end
+
+    context 'when color is provided' do
+      it 'sets the color class' do
+        expect(gl_loading_icon(color: 'light').to_s).to match 'gl-spinner-light'
+      end
+    end
+
+    context 'when inline is true' do
+      it 'creates an inline container' do
+        expect(gl_loading_icon(inline: true).to_s).to start_with '<span class="gl-spinner-container"'
       end
     end
   end
-- 
GitLab