Display image credit for lead images #1190

leenagupte · 2018-12-18T15:42:22Z

Trello: https://trello.com/c/hGmJ5BfV
Related to:

Motivation

Content Publisher allows publishers to add an image credit, but we are not displaying this data on the frontend

Expected changes

Desktop

Mobile

Paired with @DilwoarH

alex-ju · 2018-12-18T16:02:35Z

app/views/components/_figure.html.erb

@@ -9,4 +10,10 @@
      <%= caption %>
    </figcaption>
  <% end %>
+
+  <% if credit.present? %>
+    <figcredit class="app-c-figure__figcredit">


I assume this wasn't meant to be custom element but rather part of the <figcaption> or a separate <span>?

What is figcaption? We were trying to follow the pattern defined by captions, but weren't sure where this element is defined.

Should we just make it a span?

<figcaption> is a defined element representing a caption or a legend associated with an image/figure. They must be placed inside a <figure> element to be semantically correct. There is no html element for credits though, so we can use a generic tag (i.e. <span> or we can include the credits inside the <figcaption> element.

TIL! I thought figcaption was something we'd made up!

I'll use figcaption for consistency.

As @vanitabarrett mentioned, <figure> should only have one <figcaption> so it probably makes more sense to be included in the existing tag.

vanitabarrett · 2018-12-19T11:45:45Z

@leenagupte Would it make more sense to just include the credit as part of the original image figcaption element? E.g:

<figcaption class="app-c-figure__figcaption">
      <%= caption %>

     <% if credit.present? %>
          <p>Image credit: <%= credit %></p>
     <% end %>
    </figcaption>
  <% end %>

vanitabarrett

I'm not sure about having 2 figcaption elements when there is both a caption and a credit. I think it would make more sense to include the credit text inside the figcaption?

leenagupte · 2018-12-19T13:28:25Z

@vanitabarrett @alex-ju I've made the changes you recommended and fixed-up.

vanitabarrett · 2018-12-19T13:31:52Z

app/assets/stylesheets/components/_figure.scss

+.app-c-figure__figcaption-text {
+  margin-bottom: $gutter-one-third;
+
+  @include media(mobile) {


We normally try and do mobile-first development, which means our CSS should be based on mobile devices and we should add any other styling for larger screen sizes in media queries. So in this case, it would mean swapping round the CSS you've added here.

@vanitabarrett Updated and fixed up.

vanitabarrett · 2018-12-19T13:33:25Z

app/views/components/_figure.html.erb

    <figcaption class="app-c-figure__figcaption">
-      <%= caption %>
+      <% unless caption.blank? %>
+        <div class="app-c-figure__figcaption-text">


Could we use paragraph tags here instead of divs? For both app-c-figure__figcaption-text and app-c-figure__figcaption-credit

Sure! Not sure what the difference re: <p> and <div> is to be honest.

Visually, they probably look the same unless you add your own styling. It's more about the semantics - a <p> (paragraph tag) is designed to wrap text, whereas a <div> is just a general containing element (which we tend to use for grouping or layout). Using semantic HTML means we are properly describing what the document/page is made up of: a load of div elements won't tell you a lot, but things like p, image, a etc give you more information.

I hope that makes sense! 🙂

@vanitabarrett updated and fixed up.

vanitabarrett · 2018-12-19T13:33:46Z

app/views/components/_figure.html.erb

+
+      <% if credit.present? %>
+          <div class="app-c-figure__figcaption-credit">
+            Image credit: <%= credit %>


Should "Image credit" come from a translation file?

Do you know what happens if there isn't a translation available for the other languages?

I thought it fell back to the english one... but you can provide a default too?

You're right.

The I18n library will use English as a default locale, i.e. if a different locale is not set, :en will be used for looking up translations.

From: https://guides.rubyonrails.org/i18n.html

@vanitabarrett I've added a translation.

@vanitabarrett Updated and fixed up.

Adds a margin to the bottom of the caption in desktop view. This works as a line-break between the caption and in the image credit. The extra line isn't displayed on mobiles as this follows the pattern being developed for displaying image credits on inline images

vanitabarrett · 2018-12-19T14:28:14Z

app/views/components/_figure.html.erb

@@ -14,7 +14,7 @@

      <% if credit.present? %>
          <p class="app-c-figure__figcaption-credit">
-            Image credit: <%= credit %>
+            <%= I18n.t("components.figure.image_credit") %>: <%= credit %>


You could make the credit part of the translation string. E.g: I18n.t("component.figure.image_credit, credit: credit) and then in the translation file something like image_credit: "Image credit: %{credit}"

But that's not necessary, this is fine too 👍

I like that better!

The translations default to English if there isn't an entry for a locale.

Content Items affected: * Case Study * Fatality Notice * News Articles * Speech * Take Part * World Location News Article At the moment only the Content Publisher app supports image credits, so they will only be displayed on news articles for the time being.

vanitabarrett · 2018-12-19T14:40:33Z

Looks great! 💯

benthorner temporarily deployed to government-frontend-pr-1190 December 18, 2018 15:42 Inactive

leenagupte changed the title ~~Add image credit to news~~ Display image credit for lead images Dec 18, 2018

leenagupte requested review from kevindew and alex-ju December 18, 2018 15:46

alex-ju reviewed Dec 18, 2018

View reviewed changes

benthorner temporarily deployed to government-frontend-pr-1190 December 18, 2018 16:17 Inactive

leenagupte force-pushed the add-image-credit-to-news branch from 97ab837 to d9e8ed1 Compare December 18, 2018 16:31

benthorner temporarily deployed to government-frontend-pr-1190 December 18, 2018 16:31 Inactive

leenagupte force-pushed the add-image-credit-to-news branch from d9e8ed1 to 3d17a39 Compare December 18, 2018 16:35

benthorner temporarily deployed to government-frontend-pr-1190 December 18, 2018 16:35 Inactive

vanitabarrett suggested changes Dec 19, 2018

View reviewed changes

leenagupte force-pushed the add-image-credit-to-news branch from 3d17a39 to 82b95cf Compare December 19, 2018 13:27

benthorner temporarily deployed to government-frontend-pr-1190 December 19, 2018 13:28 Inactive

leenagupte force-pushed the add-image-credit-to-news branch from 82b95cf to 778ec0f Compare December 19, 2018 13:30

benthorner temporarily deployed to government-frontend-pr-1190 December 19, 2018 13:30 Inactive

vanitabarrett suggested changes Dec 19, 2018

View reviewed changes

leenagupte force-pushed the add-image-credit-to-news branch from 778ec0f to 54ab422 Compare December 19, 2018 14:07

benthorner temporarily deployed to government-frontend-pr-1190 December 19, 2018 14:07 Inactive

vanitabarrett approved these changes Dec 19, 2018

View reviewed changes

leenagupte force-pushed the add-image-credit-to-news branch from 54ab422 to e995617 Compare December 19, 2018 14:25

benthorner temporarily deployed to government-frontend-pr-1190 December 19, 2018 14:26 Inactive

vanitabarrett reviewed Dec 19, 2018

View reviewed changes

leenagupte added 2 commits December 19, 2018 14:36

Move Image credit text to translation file

b0f2b1f

The translations default to English if there isn't an entry for a locale.

leenagupte force-pushed the add-image-credit-to-news branch from e995617 to 9cfd024 Compare December 19, 2018 14:39

benthorner deployed to government-frontend-pr-1190 December 19, 2018 14:39 View deployment

leenagupte merged commit 8c80471 into master Dec 19, 2018

leenagupte deleted the add-image-credit-to-news branch December 19, 2018 15:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Display image credit for lead images #1190

Display image credit for lead images #1190

leenagupte commented Dec 18, 2018 •

edited

Loading

alex-ju Dec 18, 2018

leenagupte Dec 18, 2018

leenagupte Dec 18, 2018

alex-ju Dec 18, 2018

leenagupte Dec 18, 2018

alex-ju Dec 19, 2018 •

edited

Loading

vanitabarrett commented Dec 19, 2018 •

edited

Loading

vanitabarrett left a comment

leenagupte commented Dec 19, 2018

vanitabarrett Dec 19, 2018

leenagupte Dec 19, 2018

vanitabarrett Dec 19, 2018

leenagupte Dec 19, 2018

vanitabarrett Dec 19, 2018 •

edited

Loading

vanitabarrett Dec 19, 2018

leenagupte Dec 19, 2018

vanitabarrett Dec 19, 2018

leenagupte Dec 19, 2018

vanitabarrett Dec 19, 2018

leenagupte Dec 19, 2018

leenagupte Dec 19, 2018

leenagupte Dec 19, 2018

vanitabarrett Dec 19, 2018

leenagupte Dec 19, 2018

vanitabarrett commented Dec 19, 2018

Display image credit for lead images #1190

Display image credit for lead images #1190

Conversation

leenagupte commented Dec 18, 2018 • edited Loading

Motivation

Expected changes

Desktop

Mobile

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex-ju Dec 19, 2018 • edited Loading

Choose a reason for hiding this comment

vanitabarrett commented Dec 19, 2018 • edited Loading

vanitabarrett left a comment

Choose a reason for hiding this comment

leenagupte commented Dec 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vanitabarrett Dec 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vanitabarrett commented Dec 19, 2018

leenagupte commented Dec 18, 2018 •

edited

Loading

alex-ju Dec 19, 2018 •

edited

Loading

vanitabarrett commented Dec 19, 2018 •

edited

Loading

vanitabarrett Dec 19, 2018 •

edited

Loading