Pull high resolution image URL for meta tags #4767
Conversation
yndajas
force-pushed
the
test-image-caption
branch
from
498d939 to
52fddb2
Compare
mike3985
left a comment
mike3985
left a comment
There was a problem hiding this comment.
👍 nice approach to the problem of the existing meta tags shared examples being so specific to Content Store
yndajas
force-pushed
the
test-image-caption
branch
from
52fddb2 to
337b953
Compare
hannako
left a comment
hannako
left a comment
There was a problem hiding this comment.
Looks good to me!
Perhaps you could join up the dots for how high_resolution_url is used in either the commit message or PR description. ie
https://github.com/alphagov/govuk_publishing_components/blob/a48903d9b24dc47b1434ee89200c8077bdc6bf48/lib/govuk_publishing_components/presenters/machine_readable/page.rb#L35
frontend/app/views/news_article/show.html.erb
Lines 3 to 6 in 50901ed
yndajas
force-pushed
the
test-image-caption
branch
from
337b953 to
24a75de
Compare
leenagupte
left a comment
leenagupte
left a comment
There was a problem hiding this comment.
This is looking good.
However, I agree with Jess's suggestion about adding how high resolution images work and why these tests needed be to the commit message and PR description.
There's also a minor inline suggestion.
This is [used by govuk_publishing_components][image_url] to populate the `og:image` and `twitter:image` meta tags if it's present. When absent, the standard URL of the image will be used. In order to provide consistency between when the data comes from Content Store versus GraphQL, we need to include the field in the GraphQL query This adds a test covering this for both the Content Store and GraphQL contexts. There is a similar shared example in spec/support/meta_tags.rb, but it wasn't being used in this spec before, and the approach here allows us to use a short shared example for both contexts [image_url]: https://github.com/alphagov/govuk_publishing_components/blob/a48903d9b24dc47b1434ee89200c8077bdc6bf48/lib/govuk_publishing_components/presenters/machine_readable/page.rb#L34-L36
|
@hannako @leenagupte re: joining the dots, I've added a link to the method in govuk_publishing_components in the commit message to make it more explicit, per Jess' suggestion |
leenagupte
left a comment
leenagupte
left a comment
There was a problem hiding this comment.
Thanks for expanding the commit message 👍
This is used by govuk_publishing_components to populate the
og:imageandtwitter:imagemeta tags if it's present. When absent, the standard URL of the image will be used. In order to provide consistency between when the data comes from Content Store versus GraphQL, we need to include the field in the GraphQL queryTrello