Refactor UploadedImagesComponent #8440
Conversation
davidgisbey
force-pushed
the
refactor-uploaded_images_component
branch
5 times, most recently
from
396b228 to
8b16533
Compare
|
|
||
| <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| %> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
| attr_reader :edition | ||
|
|
||
| def lead_image_guidance | ||
| if edition.type == "CaseStudy" |
There was a problem hiding this comment.
| if edition.type == "CaseStudy" | |
| if case_study? |
| else | ||
| edition.images.find_index(image) | ||
| end | ||
| end |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
We could do something like this instead.
There's 2 subtly different needs for an index here.
- we need the index of the image with the lead image included so the markdown index is correct
- 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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Okay cool thanks. Yea i think that sounds right too
davidgisbey
force-pushed
the
refactor-uploaded_images_component
branch
from
8b16533 to
a9263ce
Compare
davidgisbey
force-pushed
the
use-lead-image-association-throughout-codebase
branch
from
4109ad8 to
83361e2
Compare
At the moment, the uploaded_images_component is doing quite a lot and is tricky to parse. This pulls out the lead image functionality into it's own view component, refactors it and unit tests it a bit more thoroughly.
This follows on from the previous commit and pulls a bunch more logic out of the uploaded_images_component into another component. I've added a few methods to the edition & custom_lead_image models rather than have them within the view component itself as they seem to fit their slightly better.
davidgisbey
force-pushed
the
refactor-uploaded_images_component
branch
from
a9263ce to
6e3e9fb
Compare
davidgisbey
force-pushed
the
refactor-uploaded_images_component
branch
3 times, most recently
from
98fc488 to
6e3e9fb
Compare
Description
At the moment, the UploadedImagesComponent does a lot. It handles rendering the lead image and associated guidance, in addition to all the images that can be used inline within the body.
This adds two additional components which handle rendering the lead image and other images. I've added a bunch of unit tests for both. There's definitely some duplication, particularly around displaying the image itself, but overall i think it's the lesser evil.
Trello card
https://trello.com/c/5vCiIxeQ/1064-lead-image-handling-backend-work
Follow these steps if you are doing a Rails upgrade.