From fe98d1dabc4189ed8a9fb6000a80e7832d1f2dc5 Mon Sep 17 00:00:00 2001
From: Andy Sellick <andy.sellick@digital.cabinet-office.gov.uk>
Date: Tue, 9 Jul 2024 09:13:04 +0100
Subject: [PATCH] Fix figure component spacing

- on mobile, if a figure had a caption there was no space between it and the following content
- however if there was no caption, there was a space, because the empty figcaption element was still being rendered, which included spacing at the top (to prevent it from being too close to the image)
- this change fixes this problem and refactors the styling of the component in the following ways
- document the undocumented feature of an image credit beneath the caption
- only show the figcaption element if there is a caption or a credit
- add spacing between caption and credit when present (haven't found any live examples of this, but can be passed on case study pages)
- stop using the responsive mixin for component margin, as confusing and unnecessary
- instead explicitly set margin bottom on the component to be the same for all screen sizes (it's a big space on desktop, looks fine everywhere else too)
- retain the margin 0 for the component, as by default browsers give figure elements margin all around
- increase the font size of the caption and credit using the govuk-font mixin (it was 14 on desktop, which is too small - it's still 14px on mobile, but this will automatically be increased to 16 when govuk-frontend v5 is rolled out)
---
 .../stylesheets/components/_figure.scss       | 14 ++++++------
 app/views/components/_figure.html.erb         | 22 ++++++++++---------
 app/views/components/docs/figure.yml          |  9 ++++++++
 test/components/figure_test.rb                |  4 ++--
 4 files changed, 30 insertions(+), 19 deletions(-)

diff --git a/app/assets/stylesheets/components/_figure.scss b/app/assets/stylesheets/components/_figure.scss
index 7d11a5871..e7d7973d0 100644
--- a/app/assets/stylesheets/components/_figure.scss
+++ b/app/assets/stylesheets/components/_figure.scss
@@ -1,13 +1,16 @@
 @import "govuk_publishing_components/individual_component_support";
-@import "../mixins/margins";
 
 .app-c-figure {
   @include govuk-clearfix;
-  @include responsive-bottom-margin;
 
   border-top: 1px solid $govuk-border-colour;
   padding-top: govuk-spacing(3);
   margin: 0;
+  margin-bottom: govuk-spacing(8);
+
+  @include govuk-media-query($until: tablet) {
+    margin-bottom: govuk-spacing(6);
+  }
 }
 
 .app-c-figure__image {
@@ -28,7 +31,6 @@
 
     display: block;
     vertical-align: top;
-
     box-sizing: border-box;
     float: left;
     padding-left: govuk-spacing(3);
@@ -41,9 +43,7 @@
 }
 
 .app-c-figure__figcaption-text {
+  @include govuk-font(16);
   margin: 0;
-
-  @include govuk-media-query($from: tablet) {
-    margin-bottom: govuk-spacing(2);
-  }
+  margin-bottom: govuk-spacing(2);
 }
diff --git a/app/views/components/_figure.html.erb b/app/views/components/_figure.html.erb
index f33ee1b89..d0a100e82 100644
--- a/app/views/components/_figure.html.erb
+++ b/app/views/components/_figure.html.erb
@@ -8,16 +8,18 @@
   <% if src.present? %>
     <img class="app-c-figure__image" src="<%= src %>" alt="<%= alt %>">
   <% end %>
-  <figcaption class="app-c-figure__figcaption">
-    <% if caption.present? %>
-      <p class="app-c-figure__figcaption-text">
-        <%= caption %>
-      </p>
-    <% end %>
-    <% if credit.present? %>
-        <p class="app-c-figure__figcaption-credit">
+  <% if caption.present? || credit.present? %>
+    <figcaption class="app-c-figure__figcaption">
+      <% if caption.present? %>
+        <p class="app-c-figure__figcaption-text">
+          <%= caption %>
+        </p>
+      <% end %>
+      <% if credit.present? %>
+        <p class="app-c-figure__figcaption-text">
           <%= I18n.t("components.figure.image_credit", credit: credit) %>
         </p>
-    <% end %>
-  </figcaption>
+      <% end %>
+    </figcaption>
+  <% end %>
 </figure>
diff --git a/app/views/components/docs/figure.yml b/app/views/components/docs/figure.yml
index 9c797e76d..7e39361a6 100644
--- a/app/views/components/docs/figure.yml
+++ b/app/views/components/docs/figure.yml
@@ -32,3 +32,12 @@ examples:
       caption: 'تأشيرات'
     context:
       right_to_left: true
+  figure_with_credit:
+    data:
+      src: 'https://assets.publishing.service.gov.uk/government/uploads/system/uploads/feature/image/54374/s630_the_lords_chamber.jpg'
+      credit: Wallis Crankled
+  figure_with_caption_and_credit:
+    data:
+      src: 'https://assets.publishing.service.gov.uk/government/uploads/system/uploads/feature/image/54374/s630_the_lords_chamber.jpg'
+      caption: The Lords Chamber
+      credit: Wallis Crankled
diff --git a/test/components/figure_test.rb b/test/components/figure_test.rb
index b52147321..01b8e6467 100644
--- a/test/components/figure_test.rb
+++ b/test/components/figure_test.rb
@@ -33,7 +33,7 @@ def component_name
     render_component(src: "/image", alt: "image alt text", credit: "Creative Commons")
     assert_select ".app-c-figure__image[src=\"/image\"]"
     assert_select ".app-c-figure__image[alt=\"image alt text\"]"
-    assert_select ".app-c-figure__figcaption .app-c-figure__figcaption-credit", text: "Image credit: Creative Commons"
+    assert_select ".app-c-figure__figcaption .app-c-figure__figcaption-text", text: "Image credit: Creative Commons"
   end
 
   test "renders a figure with caption and credit correctly" do
@@ -41,6 +41,6 @@ def component_name
     assert_select ".app-c-figure__image[src=\"/image\"]"
     assert_select ".app-c-figure__image[alt=\"image alt text\"]"
     assert_select ".app-c-figure__figcaption .app-c-figure__figcaption-text", text: "This is a caption"
-    assert_select ".app-c-figure__figcaption .app-c-figure__figcaption-credit", text: "Image credit: Creative Commons"
+    assert_select ".app-c-figure__figcaption .app-c-figure__figcaption-text", text: "Image credit: Creative Commons"
   end
 end