Landing page images from content item #4375
Conversation
- Because we might now get images from the content item or locally-supplied, we check the url is a url - if it is, we return it, if not, we try to make an image_path for it and return that.
KludgeKML
force-pushed
the
landing-page-hero-asset-images
branch
from
daa6f77
to
9eca4eb
Compare
| "tablet" => "landing_page/tablet.jpeg", | ||
| "tablet_2x" => "landing_page/tablet_2x.jpeg", | ||
| }, | ||
| "sources" => [ |
There was a problem hiding this comment.
I could probably do a much better job of structuring this payload on the whitehall side. All the way down to this if we wanted:
"image" => {
"alt" => "some alt text",
"sources" => {
"hero_desktop_1x" => "landing_page/desktop.jpeg",
"hero_desktop_2x" => "landing_page/desktop_2x.jpeg",
"hero_tablet_1x" => "landing_page/tablet.jpeg",
"hero_tablet_2x" => "landing_page/tablet_2x.jpeg",
"hero_mobile_1x" => "landing_page/mobile.jpeg",
"hero_mobile_2x" => "landing_page/mobile_2x.jpeg",
},
... and whitehall could validate that the alt text is nil or the same for all of the images etc.
The other metadata (created_at, caption etc.) is always going to be a bit confusing while we're uploading three separate images (mobile, tablet, desktop) with 2 variants each.
alphagov/whitehall#9579 does produce the payload in the current format - I haven't attempted to improve it yet. I'm happy to stick with the current format if you like, but I'd probably vote to improve it before the two PRs merge.
There was a problem hiding this comment.
Okay, let's simplify it. I was hoping that maybe leaving the code the way it is would give us some more general support in the future, but I'm forgetting YAGNI. Let's make the code simpler now, and we can complicate it later if we need to.
There was a problem hiding this comment.
I'll confirm what the structure will be once I've updated my second PR
There was a problem hiding this comment.
There was a problem hiding this comment.
Hmm, are we losing the alt text entirely?
There was a problem hiding this comment.
I've updated, but kept the top-level alt.
There was a problem hiding this comment.
Yeah - I'm pulling the alt text out and setting at the top level only, so that frontend doesn't have to handle the "what if different images have different alt texts" edge case.
In actual fact, Whitehall doesn't seem to let people provide alt text for new images, so we might have to have a think about whether hero images should have alt text, or whether they're purely decorative. I'm on the fence personally.
- this will replace individual image handling in hero and featured blocks. Tests to be added in the commits for the consuming block types. - to simplify calls, we load the sources into an OpenStruct. This is close to how they're currently handled in the blocks, but removes the Dataclass strictness (which allows this to be used more generally).
- Update tests and fixtures (local files to be updated in a later commit)
KludgeKML
force-pushed
the
landing-page-hero-asset-images
branch
from
9eca4eb
to
ea8ca2a
Compare
KludgeKML
force-pushed
the
landing-page-hero-asset-images
branch
from
ea8ca2a
to
83e008f
Compare
| end | ||
|
|
||
| def load_image | ||
| FeaturedImage.new(alt: data["image"]["alt"], sources: OpenStruct.new(data["image"]["sources"])) |
There was a problem hiding this comment.
| FeaturedImage.new(alt: data["image"]["alt"], sources: OpenStruct.new(data["image"]["sources"])) | |
| FeaturedImage.new(alt: data.dig("image", "alt"), sources: OpenStruct.new(data.dig("image", "sources"))) |
| <%= tag.source srcset: "#{image_path(block.image.sources.tablet)}, #{image_path(block.image.sources.tablet_2x)} 2x", media: "(min-width: 641px) and (max-width: 768px)" %> | ||
| <%= tag.source srcset: "#{image_path(block.image.sources.mobile)}, #{image_path(block.image.sources.mobile_2x)} 2x", media: "(max-width: 640px)" %> | ||
| <%= image_tag(block.image.sources.desktop, alt: block.image.alt, class: "app-b-hero__image") %> | ||
| <%= tag.source srcset: "#{block_image_path(block.image.sources.hero_desktop_1x)}, #{block_image_path(block.image.sources.hero_desktop_2x)} 2x", media: "(min-width: 769px)" %> |
There was a problem hiding this comment.
Currently my branch drops the hero_ prefix from the sources hash, so they're just desktop_2x etc. That was just because it felt like duplication to be doing - type: "hero" and then reiterating hero_ a bunch of times. But it does introduce an inconsistency with the version names configured in whitehall, as those have to be globally unique, so they do need prefixes.
I don't mind either way really - I'd be happy to update my branch to put back the hero_s if you think its more consistent to have them. Or we could update them here.
There was a problem hiding this comment.
🤔 actually maybe I can get it to match whats currently there exactly, and we can lose this PR 🤔
| @@ -35,14 +35,14 @@ blocks: | |||
| navigation_group_id: Top Menu | |||
| - type: hero | |||
| image: | |||
| alt: "Placeholder alt text" | |||
| alt_text: "Placeholder alt text" | |||
There was a problem hiding this comment.
Ah, I didn't mean to change this from alt to alt_text 😓
Hero and Featured blocks will now take their image sets from the content item, and can understand the difference between hardcoded images that have to be pathed and remote images where the url can be used directly.
Related to: alphagov/whitehall#9579
https://trello.com/c/00bwdpYL/124-images-from-whitehall