Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor UploadedImagesComponent #8440

Merged
merged 2 commits into from
Nov 2, 2023
Merged

Refactor UploadedImagesComponent #8440

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fb8c641
Add lead image view component
davidgisbey Nov 1, 2023
6e3e9fb
Add image view component
davidgisbey Nov 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions app/components/admin/edition_images/image_component.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
<li class="govuk-grid-row">
<div class="govuk-grid-column-one-third">
<img src="<%= image.url %>" alt="<%= preview_alt_text %>" class="app-view-edition-resource__preview">
<% unless image.image_data&.all_asset_variants_uploaded? %>
<span class="govuk-tag govuk-tag--green">Processing</span>
<% end %>
</div>

<div class="govuk-grid-column-two-thirds">
<p class="govuk-body"><strong>Caption: </strong><%= caption %></p>
<p class="govuk-body"><strong>Alt text: </strong><%= alt_text %></p>
<%= render "govuk_publishing_components/components/copy_to_clipboard", {
label: tag.strong("Markdown code:"),
copyable_content: image_markdown,
button_text: "Copy Markdown",
} %>
</div>

<div class="app-view-edition-resource__actions govuk-grid-column-full govuk-button-group">
<% links.each do |link| %>
<%= link %>
<% end %>
</div>
</li>

<% unless last_image %>
<li aria-hidden="true"><hr class="app-view-edition-resource__section-break govuk-section-break govuk-section-break--visible"></li>
<% end %>
48 changes: 48 additions & 0 deletions app/components/admin/edition_images/image_component.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
# frozen_string_literal: true

class Admin::EditionImages::ImageComponent < ViewComponent::Base
def initialize(edition:, image:, last_image:)
@edition = edition
@image = image
@last_image = last_image
end

private

attr_reader :edition, :image, :last_image

def preview_alt_text
"Image #{find_index_from_non_lead_images + 1}"
end

def caption
image.caption.presence || "None"
end

def alt_text
image.alt_text.presence || "None"
end

def image_markdown
edition.images_have_unique_filenames? ? "[Image: #{image.filename}]" : "!!#{find_image_index + 1}"
end

def find_index_from_non_lead_images
if edition.respond_to?(:non_lead_images)
edition.non_lead_images.find_index(image)
else
edition.images.find_index(image)
end
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand this correctly, it's required because the document_images method on the parent component has excluded the lead image from the images list, right? I haven't really thought this through and it's probably not a good idea, but would setting the value of the list item to nil instead of removing it altogether make this slightly less clunky? Or is that just going to be really confusing?

Copy link
Contributor Author

@davidgisbey davidgisbey Nov 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This methods changed slightly actually. If we had access to the document_images method then we could just use that here.

We could potentially inherit from the uploaded images component. Or we could pass the document_images in as an additional parameter too.

There is a method below that still calls edition.images.find_index(image) which is used for populating the legacy index markdown. In that case i guess we could nil the image in document_images but then we have to introduce additional code to compact the array in specific situations right?

Copy link
Contributor Author

@davidgisbey davidgisbey Nov 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do you think about doing this ead27d5?

I know a lot of us are quite inheritance averse, but currently this only provides the document_images method and i can't see a reason this would be nested any deeper for the foreseeable future.

Copy link
Contributor

@ryanb-gds ryanb-gds Nov 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not keen on that, those components are not really an extension of each other. Could we get the index before we render the component somehow and pass it in as a parameter?

Copy link
Contributor Author

@davidgisbey davidgisbey Nov 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could do something like this instead.

There's 2 subtly different needs for an index here.

  1. we need the index of the image with the lead image included so the markdown index is correct
  2. we need the index of the image with the lead image excluded so we add the preview alt text for non-lead images sequentially

We def could do something like you said and set the lead image to nil in the document_images, but unless we pass the array through to the component i don't think it resolves the issue unless i'm missing something.

We could pass two indexes through but i'm not convinced we're really reducing any complexity by doing that. Wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think how it is at the moment looks better. In an ideal world perhaps the ImageComponent would not need to have access to the edition, but I don't see it as a major problem and I don't think that's very realistic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay cool thanks. Yea i think that sounds right too


def find_image_index
edition.images.find_index(image)
end

def links
links = []
links << link_to("Edit details", edit_admin_edition_image_path(@edition, image), class: "govuk-link")
links << link_to("Delete image", confirm_destroy_admin_edition_image_path(@edition, image), class: "govuk-link gem-link--destructive")
links
end
end
55 changes: 55 additions & 0 deletions app/components/admin/edition_images/lead_image_component.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
<%= render "govuk_publishing_components/components/heading", {
text: "Lead image",
font_size: "m",
padding: true,
} %>

<%= render "govuk_publishing_components/components/details", {
title: "Using a lead image",
} do %>
<% lead_image_guidance %>
<% end %>

<% if lead_image.present? || case_study? %>
<div class="govuk-grid-row">
<% if lead_image.present? %>
<div class="app-c-edition-images-lead-image-component__lead_image">
<div class="govuk-grid-column-one-third">
<img src="<%= lead_image.url %>" alt="Lead image" class="app-view-edition-resource__preview">
<% unless lead_image.image_data&.all_asset_variants_uploaded? %>
<span class="govuk-tag govuk-tag--green">Processing</span>
<% end %>
</div>

<div class="govuk-grid-column-two-thirds">
<p class="govuk-body"><strong>Caption: </strong><%= caption %></p>
<p class="govuk-body"><strong>Alt text: </strong><%= alt_text %></p>
</div>
</div>
<% end %>

<div class="app-view-edition-resource__actions govuk-grid-column-full govuk-button-group">
<% if case_study? %>
<%= form_with(url: update_image_display_option_admin_edition_path(edition), method: :patch) do |form| %>
Copy link
Contributor Author

@davidgisbey davidgisbey Nov 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

previously these links were wrapped in the form regardless of whether or not it was a case study. Oo there was a bit less duplication, but the form rendered regardless of whether it could be submitted/was relevant to the edition type.

This definitely adds some duplication though. Interested to see what people think.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do the links need to be inside the form when it is a case study? It looks like they should always render when the lead image is present, so could they just go outside of the case study condition? Appreciate we might have to tweak some styles

<%= hidden_field_tag "edition[image_display_option]", new_image_display_option %>

<%= render "govuk_publishing_components/components/button", {
text: update_image_display_option_button_text,
secondary_solid: true,
margin_bottom: 4,
} %>

<% if lead_image.present? %>
<% links.each do |link| %>
<%= link %>
<% end %>
<% end %>
<% end %>
<% elsif lead_image.present? %>
<% links.each do |link| %>
<%= link %>
<% end %>
<% end %>
</div>
</div>
<% end %>
86 changes: 86 additions & 0 deletions app/components/admin/edition_images/lead_image_component.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
# frozen_string_literal: true

class Admin::EditionImages::LeadImageComponent < ViewComponent::Base
def initialize(edition:)
@edition = edition
end

def render?
edition.can_have_custom_lead_image?
end

private

attr_reader :edition

def lead_image_guidance
if case_study?
tag.p("Using a lead image is optional and can be shown or hidden. The first image you upload is used as the lead image.", class: "govuk-body") + tag.p("The lead image appears at the top of the document. The same image cannot be used in the body text.", class: "govuk-body")
else
tag.p("The first image you upload is used as the lead image.", class: "govuk-body") + tag.p("The lead image appears at the top of the document. The same image cannot be used in the body text.", class: "govuk-body")
end
end

def case_study?
edition.type == "CaseStudy"
end

def lead_image
@lead_image ||= edition.lead_image
end

def caption
lead_image.caption.presence || "None"
end

def alt_text
lead_image.alt_text.presence || "None"
end

def new_image_display_option
@new_image_display_option ||= if image_display_option_is_no_image? && edition_has_images?
"custom_image"
elsif image_display_option_is_no_image?
"organisation_image"
else
"no_image"
end
end

def image_display_option_is_no_image?
edition.image_display_option == "no_image"
end

def update_image_display_option_button_text
return image_display_option_button_text_when_image_has_been_uploaded if edition_has_images?

image_display_option_button_text_when_no_images_uploaded
end

def image_display_option_button_text_when_image_has_been_uploaded
return "Hide lead image" if new_image_display_option_is_no_image?

"Show lead image"
end

def image_display_option_button_text_when_no_images_uploaded
return "Remove lead image" if new_image_display_option_is_no_image?

"Use default image"
end

def new_image_display_option_is_no_image?
new_image_display_option == "no_image"
end

def edition_has_images?
edition.images.present?
end

def links
links = []
links << link_to("Edit details", edit_admin_edition_image_path(edition, lead_image), class: "govuk-link")
links << link_to("Delete image", confirm_destroy_admin_edition_image_path(edition, lead_image), class: "govuk-link gem-link--destructive")
links
end
end
71 changes: 3 additions & 68 deletions app/components/admin/edition_images/uploaded_images_component.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,88 +3,23 @@
font_size: "l",
} %>

<% if can_have_lead_image? %>
<%= render "govuk_publishing_components/components/heading", {
text: "Lead image",
font_size: "m",
padding: true,
} %>

<%= render "govuk_publishing_components/components/details", {
title: "Using a lead image",
} do %>
<% lead_image_guidance %>
<% end %>
<% end %>

<div class="govuk-grid-row">
<% if lead_image_hash %>
<div class="govuk-grid-column-one-third">
<img src="<%= lead_image_hash[:url] %>" alt="<%= lead_image_hash[:preview_alt_text] %>" class="app-view-edition-resource__preview">
<% unless lead_image_hash[:all_image_asset_variants_uploaded] %><span class="govuk-tag govuk-tag--green">Processing</span><% end %>
</div>
<div class="govuk-grid-column-two-thirds">
<p class="govuk-body"><strong>Caption: </strong><%= lead_image_hash[:caption] %></p>
<p class="govuk-body"><strong>Alt text: </strong><%= lead_image_hash[:alt_text] %></p>
</div>
<% end %>

<% if lead_image_hash || @edition.type == "CaseStudy" %>
<%= form_with(url: update_image_display_option_admin_edition_path(@edition), method: :patch) do |form| %>
<div class="app-view-edition-resource__actions govuk-grid-column-full govuk-button-group">
<% if @edition.type == "CaseStudy" %>
<%= hidden_field_tag "edition[image_display_option]", new_image_display_option %>
<%= render "govuk_publishing_components/components/button", {
text: update_image_display_option_button_text,
secondary_solid: true,
margin_bottom: 4,
} %>
<% end %>

<% if lead_image_hash %>
<% lead_image_hash[:links].each do |link| %><%= link %><% end %>
<% end %>
</div>
<% end %>
<% end %>

</div>
<%= render Admin::EditionImages::LeadImageComponent.new(edition: edition) %>

<%= render "govuk_publishing_components/components/heading", {
text: "Images available to use in document",
font_size: "m",
padding: true,
} %>

<% if document_images %>
<% if document_images.present? %>
<%= render "govuk_publishing_components/components/inset_text", {
text: "Copy the image markdown code to add images to the document body.",
margin_top: 0,
} %>

<ul class="govuk-list">
<% document_images.each do |image| %>
<li class="govuk-grid-row">
<div class="govuk-grid-column-one-third">
<img src="<%= image[:url] %>" alt="<%= image[:preview_alt_text] %>" class="app-view-edition-resource__preview">
<% unless image[:all_image_asset_variants_uploaded] %><span class="govuk-tag govuk-tag--green">Processing</span><% end %>
</div>
<div class="govuk-grid-column-two-thirds">
<p class="govuk-body"><strong>Caption: </strong><%= image[:caption] %></p>
<p class="govuk-body"><strong>Alt text: </strong><%= image[:alt_text] %></p>
<%= render "govuk_publishing_components/components/copy_to_clipboard", {
label: tag.strong("Markdown code:"),
copyable_content: image[:markdown],
button_text: "Copy Markdown",
} %>
</div>
<div class="app-view-edition-resource__actions govuk-grid-column-full govuk-button-group">
<% image[:links].each do |link| %><%= link %><% end %>
</div>
</li>
<% if image != document_images.last %>
<li aria-hidden="true"><hr class="app-view-edition-resource__section-break govuk-section-break govuk-section-break--visible"></li>
<% end %>
<%= render Admin::EditionImages::ImageComponent.new(edition: edition, image: image, last_image: image == document_images.last) %>
<% end %>
</ul>
<% else %>
Expand Down
73 changes: 4 additions & 69 deletions app/components/admin/edition_images/uploaded_images_component.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,78 +5,13 @@ def initialize(edition:)
@edition = edition
end

def can_have_lead_image?
@edition.can_have_custom_lead_image?
end

def lead_image
@lead_image ||= can_have_lead_image? ? @edition.lead_image : nil
end

def lead_image_hash
image_to_hash(lead_image, 0) if lead_image
end

def document_images
@document_images ||= (@edition.images - [lead_image].compact)
.map.with_index(1) { |image, index| image_to_hash(image, index) }
end

def new_image_display_option
if @edition.image_display_option == "no_image" && @edition.images.present?
"custom_image"
elsif @edition.image_display_option == "no_image"
"organisation_image"
else
"no_image"
end
end

def update_image_display_option_button_text
if @edition.images.present?
return "#{new_image_display_option == 'no_image' ? 'Hide' : 'Show'} lead image"
end

"#{new_image_display_option == 'no_image' ? 'Remove lead' : 'Use default'} image"
end

def lead_image_guidance
if @edition.type == "CaseStudy"
tag.p("Using a lead image is optional and can be shown or hidden. The first image you upload is used as the lead image.", class: "govuk-body") + tag.p("The lead image appears at the top of the document. The same image cannot be used in the body text.", class: "govuk-body")
else
tag.p("The first image you upload is used as the lead image.", class: "govuk-body") + tag.p("The lead image appears at the top of the document. The same image cannot be used in the body text.", class: "govuk-body")
end
end

private

def all_image_asset_variants_uploaded?(image)
image.image_data.all_asset_variants_uploaded? if image.image_data.present?
end
attr_reader :edition

def image_to_hash(image, index)
{
url: image.url,
preview_alt_text: lead_image == image ? "Lead image" : "Image #{index}",
caption: image.caption.presence || "None",
alt_text: image.alt_text.presence || "None",
markdown: unique_names? ? "[Image: #{image.filename}]" : "!!#{index}",
links: links_for_image(image),
all_image_asset_variants_uploaded: all_image_asset_variants_uploaded?(image),
}
end

def unique_names?
return @unique_names if defined? @unique_names

names = @edition.images.map(&:filename)
@unique_names = (names.uniq.length == names.length)
end
def document_images
return edition.images unless edition.can_have_custom_lead_image?

def links_for_image(image)
links = []
links << link_to("Edit details", edit_admin_edition_image_path(@edition, image), class: "govuk-link")
links << link_to("Delete image", confirm_destroy_admin_edition_image_path(@edition, image), class: "govuk-link gem-link--destructive")
links
edition.images - [edition.lead_image].compact
end
end
Loading