Remove PfC code and consolidate onto ImageCardComponent [WHIT-3983] - #11772
Open
ChrisBAshton wants to merge 11 commits into
Open
Remove PfC code and consolidate onto ImageCardComponent [WHIT-3983]#11772ChrisBAshton wants to merge 11 commits into
ChrisBAshton wants to merge 11 commits into
Conversation
This was added in 75c3f23, when EditionImagesController made multiple calls to `image_data.file.download!`. Those calls no longer exist.
This reverts commit a973a52. This functionality was set up to distinguish between image kinds on the old Plan For Change style Landing Pages, which were never migrated to the new ImageCardComponent way of doing things. The PfC pages have since been deleted. Any other document types that have multiple image kinds (e.g. Topical Events, with header, logo and embeddable Govspeak images) use the new ImageCardComponent pattern. So we can remove this unnecessary grouping in the "Uploaded images" section (which appears below where the ImageCardComponent kinds are rendered).
This reverts most of commit de4696f, which added some logic to include a horizontal rule that was only in force on the PfC images tab.
ChrisBAshton
force-pushed
the
images-improvements
branch
4 times, most recently
from
September 8, 2026 10:49
cebf924 to
4d63409
Compare
This was only used on the PfC pages, which have now been removed. It used to be that there was a shared image uploader for every kind of image, and the user had to specify what kind of image they were uploading in order to then get the correct validation etc on that image. Now, we have a dedicated image uploader for every image kind, so this branching logic is redundant.
We're going to switch to using the ImageCardComponent when rendering the "Uploaded images" section. One thing the current ImageComponent does that ImageCardComponent does not is render the markdown embed code, and also account for legacy editions where multiple images of the same filename may have been uploaded. In this commit we've copied over the tests and logic from the ImageComponent.
This previously returned 'image' for embeddable images, the logic for which was introduced in PR #11358. Looking at the PR, the embeddable image title never actually seemed to get used - or at least it no longer seems to be used in the current iteration of the code. When we swap out the "Uploaded images" section to use this title, we'll want to call it "Embeddable image" on the top of the card, otherwise we repeat "Image" in the card title followed by "Image" as the first row of data, which looks a bit odd.
The previous code required the 'status check' element to be a direct descendent of the 'image-processing-checker' data module container, but there's no reason why we can't have the 'status check' element nested several layers deep in the DOM. This straightforward swap-out of code allows for that.
This behaviour already exists on the cropping screen and on the specialised images area (e.g. Header logo), but general embeddable images still got stuck displaying "Processing" and required users to refresh the page.
ChrisBAshton
force-pushed
the
images-improvements
branch
from
September 8, 2026 11:04
4d63409 to
c38c4d9
Compare
ChrisBAshton
changed the title
We're pointlessly maintaining two different components for rendering a summary of an image within Whitehall. It means we're also duplicating logic around checking for uploaded state, showing a 'processing label', and so on. The long term aim has always been to converge onto a single 'cards' design, and the technical pay-off in being able to delete a redundant component and its suite of tests means it is now worth making that change. The card concept has been sufficiently bedded in, as it's been the way of expressing a document's "lead" image for many months now.
This was only used on the Images tab for "Uploaded images" (embeddable images). This has now been swapped out to use the image card component.
ChrisBAshton
force-pushed
the
images-improvements
branch
from
September 8, 2026 11:31
c38c4d9 to
b1a78ed
Compare
ChrisBAshton
marked this pull request as ready for review
September 8, 2026 12:43
The ImageCardComponent is also used for Topical Event Header Image and Topical Event Logo, neither of which use the LeadImageCardComponent branch of logic in https://github.com/alphagov/whitehall/blob/e5f9e1bdbd5f852c9ff037808f97e42db780342e/app/views/admin/edition_images/index.html.erb#L24 This was causing the `<li>` to appear without any parent `<ul>` and therefore the browser's default 'bullets' to be rendered (normally suppressed with `govuk-list` class on the `<ul>`). There's no need for this to be an actual list in the markup - let's go with a div wrapping a div.
GDSNewt
approved these changes
Sep 9, 2026
GDSNewt
left a comment
GDSNewt
left a comment
Contributor
There was a problem hiding this comment.
Didn't spot anything else other that the potential issue arising from hardcoding the html id 🤓
| <% unless (idx + 1) == current_images.size %> | ||
| <li aria-hidden="true"><hr class="app-view-edition-resource__section-break govuk-section-break govuk-section-break--visible"></li> | ||
| <% end %> | ||
| <ul id="uploaded_embeddable_image_list" class="govuk-list"> |
Contributor
There was a problem hiding this comment.
If you had a document type config with
"usages": {
"example 1": {
"multiple": true,
},
"example 2": {
"multiple": true,
}
}
would it result in two divs ala
<div id="uploaded_embeddable_image_list">
</div>
...
<div id="uploaded_embeddable_image_list">
</div>
I don't think we have any config types writtenlike that currently, but could be an issue in future?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Why
We're investigating reports of images intermittently rendering. Whilst investigating that, we uncovered a fair amount of avoidable image rendering complexity, which is paid down in this PR. It doesn't solve the original issue, but it gives us a cleaner base from which to investigate futher.
Jira: https://gov-uk.atlassian.net/browse/WHIT-3983
Screenshots
This application is owned by the Whitehall Experience team. Please let us know in #govuk-whitehall-experience-tech when you raise any PRs.
Follow these steps if you are doing a Rails upgrade.