Use rummager results for image_urls #1042
Merged
Conversation
This file contains 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
| alt: document_image["alt_text"], | ||
| context: document_image["context"] | ||
| def format_document_data(documents, data_category: nil) | ||
| documents.each.with_index(1).map do |document, index| |
There was a problem hiding this comment.
I had to double check the Ruby API documentation to confirm that 1 is an offset. Could we extract this to provide a little more context?
| @@ -2,6 +2,8 @@ module Supergroups | |||
| class NewsAndCommunications < Supergroup | |||
| attr_reader :content | |||
|
|
|||
| PLACEHOLDER_IMAGE = "https://assets.publishing.service.gov.uk/government/assets/placeholder.jpg".freeze | |||
There was a problem hiding this comment.
Should we use Plek to make this environment specific?
PLACEHOLDER_IMAGE = URI::HTTPS.build(host: Plek.current.public_asset_host, path: '/government/assets/placeholder.jpg').freezeThere was a problem hiding this comment.
That's a good idea, thanks. 👍
sihugh
force-pushed
the
dont-use-content-store-for-images
branch
from
df45108 to
78a44e0
Compare
We don't need this at present because the image is part of a link. The link text overrides the alt text in this instance so we're not reducing any accessibility. This allows us to use the recently added image_url search field.
This allows us to reuse small bits of functionality
This ensures that our analytics will be consistent between sections.
This eliminates up to three calls to content store for each page's navigation. To do this we need to add image_url to the fields that rummager returns. We then need a default image in case the content doesn't have an image (not all do).
We may want to add Welsh and other translations for these later.
sihugh
force-pushed
the
dont-use-content-store-for-images
branch
from
78a44e0 to
74244e7
Compare
We're now only retrieving 3 items per section so we can delete a lot of code from the news_and_communications and services presenters and get them to return an array of results rather than hashes. This allows us to get them to behave a bit more like the other presenters.
sihugh
force-pushed
the
dont-use-content-store-for-images
branch
from
74244e7 to
c3554a4
Compare
|
I've updated this and rebased after a couple of merges. It's worth sticking |
andrewgarner
approved these changes
Aug 14, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This eliminates up to three calls to content store for each page's navigation.
To do this we need to add image_url to the fields that rummager returns.
We can remove the alt text for images that are links because the link text is more relevant (and is preferred by browsers anyway).
We then need a default image in case the content doesn't have an image (not all do).
After rebasing on top of #1043 and #1032 it became clear that we can remove some things from the
news_and_communicationsandservicesas they don't need to have content split intopromotedandtaggedcontent any more as they only display it in one form now.I've therefore removed the
promotedsections from these presenters which allows us to simplify some things and delete a bunch of code.https://government-frontend-pr-1042.herokuapp.com/apply-apprenticeship