-
Notifications
You must be signed in to change notification settings - Fork 332
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow og:image meta tag to be customisable #847
Conversation
00ef166
to
645e77a
Compare
645e77a
to
56132d5
Compare
56132d5
to
ff7a345
Compare
src/template.njk
Outdated
@@ -18,10 +18,11 @@ | |||
<link rel="apple-touch-icon" href="{{ assetPath | default('/assets') }}/images/govuk-apple-touch-icon.png"> | |||
{% endblock %} | |||
|
|||
{% block head %}{% endblock %} | |||
|
|||
{% block head %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a breaking change since before even when you set head you'd get the default open graph image.
Could this be solved by something like
<meta property="og:image" content="{{ opengraphAssetPath | default('') }}{{ assetPath | default('/assets') }}/images/govuk-opengraph-image.png">
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tru dat. updated so that the variable can just be set
ff7a345
to
254971a
Compare
src/template.njk
Outdated
{# The default og:image is added below head so that scrapers see any custom metatags first, and this is just a fallback #} | ||
<meta property="og:image" content="{{ assetPath | default('/assets') }}/images/govuk-opengraph-image.png"> | ||
{# image path needs to absolute e.g. http://wwww.domain.com/.../govuk-opengraph-image.png #} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Path refers to just the part after the domain - i.e. just the /.../govuk-opengraph-image.png
bit. How about assetUrl
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure. updated
254971a
to
12e1249
Compare
Would you mind raising an issue after this is merged on the Design System repo to include this new feature in the guidance? |
{# The default og:image is added below head so that scrapers see any custom metatags first, and this is just a fallback #} | ||
<meta property="og:image" content="{{ assetPath | default('/assets') }}/images/govuk-opengraph-image.png"> | ||
{# image url needs to be absolute e.g. http://wwww.domain.com/.../govuk-opengraph-image.png #} | ||
<meta property="og:image" content="{{ assetUrl | default('/assets') }}/images/govuk-opengraph-image.png"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the default needs to be 'assetPath' otherwise this is a breaking change still?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe {{ openGraphImagePath | assertPath | default('/assets') }}
not sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it doesn't work with assetPath anyway, so not sure it's a breaking change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point, but consider if someone set their 'assetPath' to a absolute path, it would be working for those users?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah.
we can set
{% set assetUrl = assetUrl or (assetPath | default('/assets')) %}
so if they do have it set to absolute atm, OG image will still work.
for anyone new, they can assign a new value to assetUrl
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
modified so that the default value for assetUrl
is assetPath
, insuring it is not a breaking change if anyone specified an absolute url for the assets
57b4d7a
to
062c959
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given it seems that the opengraph image needs an absolute URL, do we need to document this somewhere? e.g. https://design-system.service.gov.uk/styles/page-template/?
How did this work in GOV.UK Template?
CHANGELOG.md
Outdated
@@ -28,6 +28,15 @@ | |||
([PR #837](https://github.com/alphagov/govuk-frontend/pull/837) and | |||
[PR #848](https://github.com/alphagov/govuk-frontend/pull/848)) | |||
|
|||
- Use relative line-height |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be part of this PR.
CHANGELOG.md
Outdated
Update typography styles to use relative, unitless line-height | ||
([PR #837](https://github.com/alphagov/govuk-frontend/pull/837)) | ||
|
||
- Allow og:image meta tag to be customisable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was already customisable by modifying assetPath
– I think we need to be clearer as to what the change is here.
062c959
to
e01316d
Compare
i've updated the wording |
ae6f29f
to
6b7b7dc
Compare
Thinking about this more, if this is solved by setting 'assetPath' to an absolute url do we need to do this at all? |
we can't set the absolute path for example in the design system (as we have two different urls) |
Moving the meta tag into the `head`block so that it can be overwritten with an absolute path
6b7b7dc
to
912b9f1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this makes sense, assetUrl
feels a little vague but it works as intended, as long as we document this well in the Design System.
Moving the meta tag into the
head
block so that it can be overwritten, as the image path for it needs to be an absolute path.Fixes: #813