Skip to content
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 Open Graph image URL to be customised #2673

Merged
merged 2 commits into from
Jul 19, 2022
Merged

Allow Open Graph image URL to be customised #2673

merged 2 commits into from
Jul 19, 2022

Conversation

36degrees
Copy link
Contributor

We previously told users that if they needed to override the OpenGraph image they should add their own meta tag to the head block and that this would override the one in the template (based on the spec). Some users have found that this isn’t reliable or can result in multiple images being displayed – and it’s a waste of bytes to include a redundant meta tag.

Instead, allow the OpenGraph image URL in the template to be set using a new opengraphImageUrl Nunjucks option.

If no opengraphImageUrl is provided, keep the existing behaviour that uses assetUrl as a prefix for a non-customisable path of images/govuk-opengraph-image.png.

However, when neither opengraphImageUrl or assetUrl are passed, no longer fall back to using the assetPath option. OpenGraph image URLs must be absolute and using the assetPath only gives us a relative URL, so we should just omit the OpenGraph image tag in these cases.

This is a change in behaviour, as previously an og:image meta tag would always be included, but it only be valid if assetUrl had been set.

Fixes #1920.

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2673 June 24, 2022 16:28 Inactive
@36degrees
Copy link
Contributor Author

This will need a corresponding change to the page template guidance in the Design System, to add opengraphImageUrl to the table of options and to update the description for assetUrl.

@36degrees 36degrees changed the title Allow OpenGraph image URL to be customised Allow Open Graph image URL to be customised Jun 24, 2022
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2673 June 24, 2022 16:35 Inactive
We previously told users that if they needed to override the OpenGraph image they should add their own meta tag to the head block and that this would override the one in the template (based on the spec). Some users have found that this isn’t reliable [1] or can result in multiple images being displayed [2] – and it’s a waste of bytes to include a redundant meta tag.

Instead, allow the OpenGraph image URL in the template to be set using a new `opengraphImageUrl` Nunjucks option.

If no `opengraphImageUrl` is provided, keep the existing behaviour that uses `assetUrl` as a prefix for a non-customisable path of `images/govuk-opengraph-image.png`.

However, when neither `opengraphImageUrl` or `assetUrl` are passed, no longer fall back to using the `assetPath` option. OpenGraph image URLs must be absolute [3] and using the `assetPath` only gives us a relative URL, so we should just omit the OpenGraph image tag in these cases.

This is a change in behaviour, as previously an `og:image` meta tag would always be included, but it only be valid if `assetUrl` had been set.

Fixes #1920.

[1]: alphagov/emergency-alerts-govuk#124
[2]: #1920 (comment)
[3]: https://ogp.me/#data_types
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2673 June 27, 2022 08:26 Inactive
36degrees added a commit to alphagov/govuk-design-system that referenced this pull request Jun 27, 2022
Document the new `opengraphImageUrl` page template option, added in alphagov/govuk-frontend#2673.

Update the documentation for the `assetUrl` option to make it slightly more generic and remove the recommendation to override by adding another meta tag to the `head` block as users should use the new `opengraphImageUrl` instead.
@36degrees
Copy link
Contributor Author

Raised a PR to update the guidance for the page template: alphagov/govuk-design-system#2228

Copy link
Member

@querkmachine querkmachine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good from a tech POV.

Would the change to requiring an explicit assetUrl be considered a breaking change?

Are there situations where a team may have placed their OpenGraph image at /assets/images/govuk-opengraph-image.png explicitly because that was the default location?
Presumably they would now need to add {% set assetUrl = "/assets" %} somewhere.

Relatively minor as breaking changes go, and probably not a blocker to putting this in a feature release unless we want to be strict about it, but something to note.

@36degrees
Copy link
Contributor Author

Would the change to requiring an explicit assetUrl be considered a breaking change?

I believe this change should only impact cases where assetUrl is not set, and should only result in invalid meta tags from being included. As such, I think we should treat this as a non-breaking change.

The only exception that I can think of would be if a user has 'incorrectly' set assetPath to an absolute URL – see the bottom example.

Are there situations where a team may have placed their OpenGraph image at /assets/images/govuk-opengraph-image.png explicitly because that was the default location?
Presumably they would now need to add {% set assetUrl = "/assets" %} somewhere.

If that was the case, the current meta tag would be invalid according to the spec as it would only be a relative URL. Of course, it might be that some social sharing sites 'do the work' to try and resolve it to an absolute URL anyway – if so this would break that. But in this instance I think we're better off following the spec and only including the meta tag if we know the absolute URL.

Therefore, in order to correctly implement the Open Graph image the team would now need to either set assetUrl or the new opengraphImageUrl variable.

✅ Asset URL ✅ User-provided meta tag in head block

{% set assetUrl = "https://example.com/assets/" %}

{% block head %}
<meta property="og:image" content="https://another-example.com/whatever/images/govuk-opengraph-image.png">
{% endblock %}

Before

Two meta tags would be included:

  • <meta property="og:image" content="https://example.com/assets/images/govuk-opengraph-image.png">
  • <meta property="og:image" content="https://another-example.com/whatever/images/govuk-opengraph-image.png">

After

No change. Two meta tags would be included:

  • <meta property="og:image" content="https://example.com/assets/images/govuk-opengraph-image.png">
  • <meta property="og:image" content="https://another-example.com/whatever/images/govuk-opengraph-image.png">

✅ Asset URL ❌ User-provided meta tag in head block

{% set assetUrl = "https://example.com/assets/" %}

Before

One meta tag would be included:

  • <meta property="og:image" content="https://example.com/assets/images/govuk-opengraph-image.png">

After

No change. One meta tag would be included:

  • <meta property="og:image" content="https://example.com/assets/images/govuk-opengraph-image.png">

❌ Asset URL ✅ User-provided meta tag in head block

{% block head %}
<meta property="og:image" content="https://another-example.com/whatever/images/govuk-opengraph-image.png">
{% endblock %}

Before

Two meta tags would be included:

  • <meta property="og:image" content="/assets/images/govuk-opengraph-image.png">INVALID as the URL is not absolute
  • <meta property="og:image" content="https://another-example.com/whatever/images/govuk-opengraph-image.png">

After

One meta tag would be included:

  • <meta property="og:image" content="https://another-example.com/whatever/images/govuk-opengraph-image.png">

❌ Asset URL ❌ User-provided meta tag in head block

{# Nothing set #}

Before

One meta tag would be included:

  • <meta property="og:image" content="/assets/images/govuk-opengraph-image.png">INVALID as the URL is not absolute

After

No meta tags would be included.

❌ Asset URL ❌ User-provided meta tag in head block ✅ Asset Path

{% set assetPath = "/my-custom-path" %}

Before

One meta tag would be included:

  • <meta property="og:image" content="/my-custom-path/images/govuk-opengraph-image.png">INVALID as the URL is not absolute

After

No meta tags would be included.

❌ Asset URL ❌ User-provided meta tag in head block ⚠️ Asset Path set to absolute URL

{# This is using assetPath incorrectly as we expect it to be a path, not a URL #}
{% set assetPath = "http://example.com/assets" %}

Before

One meta tag would be included:

  • <meta property="og:image" content="http://example.com/assets/images/govuk-opengraph-image.png">

After

No meta tags would be included.

@querkmachine
Copy link
Member

Of course, it might be that some social sharing sites 'do the work' to try and resolve it to an absolute URL anyway – if so this would break that. But in this instance I think we're better off following the spec and only including the meta tag if we know the absolute URL.

I whipped up some quick tests and tried 'em on a few random services, and it seems like some do resolve relative URLs to absolute ones, including Facebook's Meta's own validator (though this was inconsistent and did complain about it when trying with two relative images).

I agree that enforcing absolute URLs is the better way to go, though.

@36degrees 36degrees merged commit 866a2ce into main Jul 19, 2022
@36degrees 36degrees deleted the opengraph-url branch July 19, 2022 14:53
36degrees added a commit that referenced this pull request Jul 21, 2022
#2673 was merged *after* v4.2.0 was released and remains an unreleased feature. There was no conflict in 536edad as the surrounding lines had not changed.
36degrees added a commit that referenced this pull request Jul 21, 2022
Move #2673 out of v4.2.0 release in change log
36degrees added a commit to alphagov/govuk-design-system that referenced this pull request Jul 22, 2022
Document the new `opengraphImageUrl` page template option, added in alphagov/govuk-frontend#2673.

Update the documentation for the `assetUrl` option to make it slightly more generic and remove the recommendation to override by adding another meta tag to the `head` block as users should use the new `opengraphImageUrl` instead.
querkmachine pushed a commit that referenced this pull request Jul 26, 2022
#2673 was merged *after* v4.2.0 was released and remains an unreleased feature. There was no conflict in 536edad as the surrounding lines had not changed.
@querkmachine querkmachine mentioned this pull request Aug 9, 2022
owenatgov pushed a commit to alphagov/govuk-design-system that referenced this pull request Aug 9, 2022
Document the new `opengraphImageUrl` page template option, added in alphagov/govuk-frontend#2673.

Update the documentation for the `assetUrl` option to make it slightly more generic and remove the recommendation to override by adding another meta tag to the `head` block as users should use the new `opengraphImageUrl` instead.
ahosgood added a commit to LandRegistry/hmlr-design-system that referenced this pull request Sep 1, 2022
* Remove ƒ typo in fieldset example

* Fix autocomplete values in multiple address example

* Add latest blog posts April 2022

* Fix dashes and punctuation

* update image to make it neater on the check a service is suitable pattern page

* update image to make it neater on the check a service is suitable pattern page and compress it to save space

* Update Node to Active LTS version v16 (Gallium)

Node.js v14 has been in maintenance mode from 19 October 2021. v16 is the active LTS version.

This in turns updates NPM to v8, meaning our lockfile has change to [`lockfileVersion` 2](https://docs.npmjs.com/cli/v8/configuring-npm/package-lock-json#lockfileversion)

This is an internal change that affects development, testing and deployment.

* Add troubleshooting documentation for Node 16, node-sass and M1 machines

If a user has previously installed locally using Node 14 and lower on their M1 machine, they will encounter `node-sass` errors when running `npm install` after pulling this commit.

This adds documentation to deal with those errors.

This should only affect developers.

* Update metalsmith to latest version (2.4.2)

* Install @metalsmith/sass

* Uninstall metalsmith-sass and node-sass

We've installed @metalsmith/sass which uses sass (Dart Sass)
under the hood instead.

* Set `quietDeps` to true and move config to @metalsmith/sass syntax

Move the existing sass config to @metalsmith/sass syntax (uses loadPaths
instead of includePaths)

Set the `quietDeps` flag to true to suppress sass  deprecation warnings for
division (/) and any future deprecations coming from GOV.UK Frontend.

* Add 'src/stylesheets' to sass loadPaths

Dart Sass wasn't able to resolve the import for 'example-init', which
we use within our component example sass.

Telling it to look in 'src/stylesheets' to resolve imports, so it can find
this file.

* Remove node-sass on node v16 troubleshooting docs

We've updated Metalsmith and moved to Dart Sass, so this issue no longer applies (see alphagov#2150)

* Update link text in examples

This aims to make the difference between the `class` and the link text clearer, particularly for people relatively new to HTML.

* Add new team member

* Make help text advice clearer in fieldset guidance

* Add placeholder text info to text input guidance

* Revert town or city field to use address-level2

* Explain issues with client side validation

* Add implementation advice to skip link guidance

* Add subheadings to text input guidance

* Replace double quotes with singles in example

* Updating error example for National Insurance number to use correct format

* adding format guidance for National Insurance number

* Update design-system-team.md.njk

Added myself to the team member list

* Update src/design-system-team.md.njk

Co-authored-by: Kimberly Grey <kimberly.grey@digital.cabinet-office.gov.uk>

* Update src/patterns/national-insurance-numbers/index.md.njk

Co-authored-by: Calvin Lau <77630796+calvin-lau-sig7@users.noreply.github.com>

* Move #top anchor to before the skip link

The ‘back to top’ link takes users to the #top anchor point, which is currently before the navigation but after the ‘skip to main content’ link.

This means you can no longer skip past the navs etc (as per WCAG 2.1 2.4.1 Bypass Blocks [1]) unless you know to press shift + tab to go backwards in the tab order.

Insetead, we want the 'back to top' link to take the user to the very top of the page, so that the next tab will take them to ‘Skip to main content’ (or the cookie banner, if it’s not been dismissed).

We also considered taking the user to the start of `#main` [2], bypassing all of the navigation, but opted instead to move the `#top` anchor to be the very first thing because:

1. The back to top link was introduced in alphagov#687, when we moved from the side navigation and the main content being fixed, independently scrollable panes. This change meant that the side navigation no longer remained within the viewport.

It seems that the intent at the time was that the back to top link would bring the side navigation back into view, rather than providing a convenient way for the user to get back to the top of the main content.

2. Linking to #main makes it harder for users who have to get back to the navigation:

> If you're using a screen reader you could jump between landmarks using hot keys, but if you're just using a keyboard you're kinda just left with the tab key. Maybe I'm missing something, but the only options I could see to get back to the nav would be:
>
> - use TAB to go all the way through the content and footer content until you eventually get back to the top of the page
> - use back to top to get to the top of the main, and then use SHIFT+TAB to reverse through the nav menu
> - refresh the page and use TAB

3. We think users would expect to see the header when using the ‘Back to top’ link, especially if they were aiming for the navigation.

[1]: https://www.w3.org/TR/WCAG21/#bypass-blocks
[2]: alphagov#1561 (comment)

Closes alphagov#1561

* Make layout name consistent throughout guidance

* Update guidance on footer component

This proposed update:

- adds links to relevant information on privacy notices, cookie pages and accessibility statements
- clarifies copyright and licensing information

Related issues:

alphagov#1225
alphagov#1306

* Update character count guidance to match new implementation

* Minor edits for consistency

* Clarify and add accessibility information

* Update What's New section for v4.1.0

* Update roadmap

* Add context to JAWS issue

* Bump govuk-frontend to 4.1.0

* Add nodeListForEach helper directly into design system

The GOV.UK Frontend ES modules don't make the common (e.g: `nodeListForEach`)
function available to import. We've made this decision intentionally as we never
really intended for `nodeListForEach` to be something we provided.

This introduces a helper.js file which contains `nodeListForEach`. Switch our
JS files to use that helper, rather than the one from GOV.UK Frontend.

* Replace individual JS imports with ES modules initAll

* Explain that red border depends on exceeding limit

* Add accurate content for email field char limit

* Replace in guidance and examples

-  error message
 - textarea
 - text input
 - character count (including error example)

* Bump actions/setup-node from v2 to v3

* Use node-version-file input to specify node version

actions/setup-node v2.5.0 and above support reading a .nvmrc file itself [[1]].
This lets us simplify our code.

[1]: https://github.com/actions/setup-node/releases/tag/v2.5.0

* Make validation guidance language more consistent

* Add info on default components in page template

* Change contact panel headings to reflect the kind of page they're on

* Make link text in example reflect guidance

* Reorder link text

* Update team page on Design System site

* Update cookie hide button to be more descriptive

Change cookie button text from 'Hide this message' to
'Hide cookie message' to provide context to any users using
assistive tech navigating out of context.

* Add Claire to team page

* Fix em-dash

* Remove last line about performance platform

Remove last line on the Payment Card details page, which refers to the performance platform. This platform no longer exists.

* add a hint to the select to help users understand the options

* Bump govuk-frontend to v4.2.0

* Add guidance for pagination component

Iterate content and add headings for examples

Fix typo for heading level

Fix headings

Add guidance for pagination component

Iterate content and add headings for examples

Fix typo for heading level

Fix headings

Apply suggestions from tech writer review

Co-authored-by: EoinShaughnessy <72507742+EoinShaughnessy@users.noreply.github.com>

Update pagination guidance

Updated guidance to:

- cover issues raised by working group
- clarify use case for different types of pagination - and when to use 'continue' button and back link instead

Minor edits for dashes and contractions

Tweaking paragraph breaks

Remove section about saying "page" in labels

By default, "Previous" and "next" do not include "page" so this section is no longer needed

* Remove pagination component from community backlog

* Update what's new and roadmap for 4.2.0

* Update roadmap

Tidying up and reorganising some of our roadmap items in mid-2022.

* Fixed typo on pagination documentation

* Add paragraph tag to link example

This link example does not include a wrapping element, meaning that some of the text is appearing as default serif text instead of in Transport. As the text above this example states that it is
regarding links at the end of sentences or paragraphs, the example shouldn't be negatively affected by the inclusion of a paragraph.

* Indent Nunjucks code

Attempt to make this file easier to read by indenting Nunjucks code similarly to HTML.

* Refactor design system tabs JS

* Fix tab not being opened when macro options are linked to

* Update test ID and result checks

* Check for existence of tabs

* Update team member

Add Ben to the team

* Remove govuk-react-jsx which is no longer supported

* Bump terser from 5.7.0 to 5.14.2

Bumps [terser](https://github.com/terser/terser) from 5.7.0 to 5.14.2.
- [Release notes](https://github.com/terser/terser/releases)
- [Changelog](https://github.com/terser/terser/blob/master/CHANGELOG.md)
- [Commits](https://github.com/terser/terser/commits)

---
updated-dependencies:
- dependency-name: terser
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>

* Bump moment from 2.29.2 to 2.29.4

Bumps [moment](https://github.com/moment/moment) from 2.29.2 to 2.29.4.
- [Release notes](https://github.com/moment/moment/releases)
- [Changelog](https://github.com/moment/moment/blob/develop/CHANGELOG.md)
- [Commits](moment/moment@2.29.2...2.29.4)

---
updated-dependencies:
- dependency-name: moment
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>

* Apply `this` binding changes from code review

Co-authored-by: Oliver Byford <oliver.byford@digital.cabinet-office.gov.uk>

* Refactor getDesktopTab to not use a loop

* Bump metalsmith to 2.5.0

* Remove ‘mainstream’ from images guidance

Instead, define non-service content by content type and publishing method.

As flagged in alphagov/govuk-design-system-backlog#70 (comment):

> Mainstream is 2500 pages managed by GDS, but this guidance also applies to all content on www.gov.uk produced by depts, the half a million 'whitehall' pages.

Co-authored-by: Calvin Lau <77630796+calvin-lau-sig7@users.noreply.github.com>

* Use the official core metalsmith postcss plugin

Metalsmith is being updated fairly regularly now, and have an [official PostCSS plugin](https://github.com/metalsmith/postcss).

We've been blocked on bumping some dependencies because of this (alphagov#2016).

I've run `npm build` and there is one small prefixing change in the resulting CSS, but otherwise it's exactly the same.

* Only toggle details aria attributes if already set

I believe this dates from when the polyfill for the `<details>` element used to be applied in all browsers, and added extra ARIA attributes which always needed to be kept in sync when this script toggled the open state.

However, since GOV.UK Frontend v3.1.0 the polyfill now does nothing in browsers that natively support the <details> elements [1].

This means that once these attributes are set on page load, unless the polyfill is running there is nothing to keep them in sync if the user toggles the details element:

- `aria-expanded` will always be `true`, potentially causing AT to announce the expanded state incorrectly
- `aria-hidden` will always be `false` – although this is likely not an issue as aria-hidden can't make things hidden using display:none 're-appear' in the accessibility tree

Instead, only toggle `aria-expanded` if it is already set on the element (which should only happen if the polyfill has run, in which case the browser does not natively support the `<details>` element).

We also need to re-introduce the `aria-hidden` attribute removed in 8b53583 as when the polyfill runs it currently adds an `aria-hidden` attribute which means the content inside the `<details>` element will be inaccessible even when opened unless we remove it.

Add a comment to try and explain why this code needs to exist.

[1]: alphagov/govuk-frontend#1523

* Bump metalsmith-env to 2.2.0

- Allow storing the environment variables under a metadata[metadatakey] instead of metadata. See opts.metadatakey for more details.
- Dependency updates

* Use @metalsmith/in-place in favour of metalsmith-in-place

Core metalsmith plugin is effectively an updated version of the previous package.

v4.5.0

- feat: org migration, core plugin alignment
- feat: use metalsmith.match instead of multimatch, drop Node < 10 support
- feat: better jsdoc typehints & defaults mgmt
- fix: don't mistake dots in folder paths for extensions

* Use @metalsmith/layouts in favour of metalsmith-layouts

The core plugin replaces the old one.

v2.5.1
- pass the transformed list of files to metalsmith.match…
- Merge pull request alphagov#184 from doteco/master
- pass the transformed list of files to metalsmith.match so that renamed files can be matched

v2.5.0

- use metalsmith.match instead of multimatch
- Remove semicolons, run formatting. Remove devDependencies
- feat: add JSdocs, named plugin function
- Dependencies: updates debug to 4.3.4
- Drop Node < 12 support

2.4.0

- Return when done
- Improve readme
- Align dotfiles, repo with core plugin setup @metalsmith
- deps: update dependencies and fix prefer-object-spread

* Use @metalsmith/permalinks in favour of metalsmith-permalinks

Core plugin replaces old package.

Changelog: https://github.com/metalsmith/permalinks/blob/master/CHANGELOG.md

* Bump metalsmith-uglify to v2.4.1

Patch bump to update some dependencies.

* Bump @metalsmith/sass from 1.0.0 to 1.0.1

Changes: metalsmith/sass@1.0.0...1.0.1

* Adjust link text to GOV.UK image guidance

* Allow start buttons to be forms

Although unusual, there are a few valid circumstances where you might
want to submit a form when pressing the start button. A situation that
came up recently was we wanted to record analytics on when a user
started a service by recording the time that a user presses the start
button, without client-side JavaScript. The implementation for this
involved using the Rails `button_to` helper which generates a form which
POSTs to a route and includes a single `submit` button.

I think this is a valid use case of the start button, so I suggest
changing the wording on this to be slightly more open to the idea of
the start button not being a link.

* Update link to task list Sass file

This file has changed location in recent changes to the Prototype Kit, this updates the URL to no longer 404.

* Override auto-linking with period character

* Move width override classes from spacing to layout

Overriding the width of an element really has nothing to do with spacing, so this guidance make more sense as part of the layout guidance, alongside the documentation about the grid system which uses the same ‘one third’, ‘one half’ concepts.

* Tweak width override guidance for new context

Remove the link to the grid system which is now on the same page and is introduced before the override classes

Remove a reference to the spacing override classes which doesn’t make sense now that they’re on a separate page and the user might not have read about them already.

* Update links to width override classes

* Remove link to archived ethnic groups pattern

The ethnic groups pattern was archived in alphagov#1582.

* Remove broken link in page template guidance

The ‘variables’ and ‘blocks’ tables were merged in alphagov#1311 but this link was missed.

* Remove redundant classes

As noted in alphagov/govuk-frontend#2733, these classes don’t actually do anything.

* Remove extra closing `</tr>` tag

* Swap small and large screen sizes in spacing table

Putting the smaller sizes on the left-hand size seems more logical and fits better with a mobile-first approach.

* Flip order of spacing scale in table

Going from the smallest size to the largest size just makes more sense.

* Fix mismatched heading tags in navigation

* Document the static spacing scale

Fixes alphagov#1210.

* Improve headings for override classes and helpers

* Move section on overrides last

Try to encourage users who are writing their own CSS to use the helpers that we provide rather than the override classes, by making it more likely they’ll read about them first.

* Update link to my talk for more accessible blog

Allows more options to access it as I've made it into a blog post too.

* Tweaks to guidance on helpers

* Tweaks to guidance on override classes

* Bump autoprefixer to 10.4.7

One major version bump. Version 10:

- Uses PostCSS 8
- Removes support for Node.js 6, 8 and 11
- PostCSS is now a peerDependency

None of which affects our installation.

* Replace sass-lint with stylelint

* Add basic stylelint config

The resulting CSS appears to be the same after running `npm run build`.

* Fix SCSS linting errors

* Update documentation

* Replace metalsmith-tagcleaner with a custom marked renderer

* Bump marked and  jstransformer packages

Makes sense to bump these as part of this work, since they're being used under the hood to transform our files.

jstransformer-nunjucks Changelog: https://github.com/jstransformers/jstransformer-nunjucks/blob/master/CHANGELOG.md

- The major version bump simply adds the option to use custom Nunjucks loaders

jstransformer-marked Changelog: https://github.com/jstransformers/jstransformer-marked/blob/master/HISTORY.md

* Apply suggestions from code review

Co-authored-by: Calvin Lau <77630796+calvin-lau-sig7@users.noreply.github.com>

* Update logo in header to match GOV.UK Frontend

Mirror the changes that have been made to the Header component in GOV.UK Frontend [1] including:

- using conditional comments for header fallback PNG rather than an `<image` element [2]
- camel-casing the SVG attributes [3]

Fixes alphagov#2280.

[1]: https://github.com/alphagov/govuk-frontend/blob/aceb8d9798d8d2fc28357f1dda62d1a5e4a2eb98/src/govuk/components/header/template.njk
[2]: alphagov/govuk-frontend#2229
[3]: alphagov/govuk-frontend#1838

* Document static spacing override classes

Static spacing override classes are being introduced to GOV.UK Frontend (alphagov/govuk-frontend#2672).

Document the new classes so that users know how they work, and how they compare to the existing responsive spacing override classes.

* Change to more up to date link

GSS appear to have moved to a new site - the link was pointing at the old (and out of date) archived standard.

* Remove examples for archived ethnic groups pattern

The ‘Ask users for ethnic groups’ pattern was archived in alphagov#1582 [1] (dd77104) when the guidance was moved into the ‘Ask users for equality information’ pattern.

New examples were created for the equality information pattern, but the existing examples and the SVG included in the guidance were not removed from the repo.

Remove the examples and the SVG previously included in the archived ethnic groups pattern.

[1]: alphagov#1582

* Use subclass for marked renderer

This better matches how things are done in Marked, and allows us to user `super` for cases other than images.

Co-authored-by: Oliver Byford <oliver.byford@digital.cabinet-office.gov.uk>

* Fix markdown lists on pagination component page

When there's no empty line before the list the Markdown renderer is
treating it like it's part of the preceding paragraph.

* Revert "Replace metalsmith-tagcleaner with a custom marked renderer"

* Update src/patterns/equality-information/index.md.njk

Co-authored-by: Calvin Lau <77630796+calvin-lau-sig7@users.noreply.github.com>

* Fix matchMedia event listener in IE / Safari < 14

IE 10/11 and Safari >= 5.1 < 14 support `window.matchMedia` but do not support `MediaQueryList.addEventListener`.

Instead we have to use the non-standard (and deprecated) `MediaQueryList.addListener`, which only takes one argument which is the callback function to run when the media query status changes [1].

Test for the presence of the `MediaQueryList.addEventListener` method and then use whichever method is appropriate for the browser.

[1]: https://developer.mozilla.org/en-US/docs/Web/API/MediaQueryList/addListener

Co-authored-by: Owen Jones <64783893+owenatgov@users.noreply.github.com>

* Enable aliases for section headings

* Alias section break as hr

* Update What's New for v4.3.0

* Update govuk-frontend to 4.3.0

* Document `opengraphImageUrl` page template option

Document the new `opengraphImageUrl` page template option, added in alphagov/govuk-frontend#2673.

Update the documentation for the `assetUrl` option to make it slightly more generic and remove the recommendation to override by adding another meta tag to the `head` block as users should use the new `opengraphImageUrl` instead.

* Retry failed tests

The "see no evil, hear no evil" approach. This does not address the flaky tests themselves, but sets `jest.retryTimes` to 3, rerunning failed tests and greatly increasing the likelihood of a CI test task passing.

This is speedier (and more automatic) than rerunning a full CI task again, so might be enough if the flakiness isn't easily addressable.

* Fix linting issues

* Update team members list

* Reference cookie banner in updating guidance

We now have a cookie banner component, so we should link to it from this guide.

* Add ‘measuring the value’ talk to community page

* Update what's new with DSDay blog post

Tweak line breaks to look better

Add full stop

* Update date

Co-authored-by: NoraGDS <57447099+NoraGDS@users.noreply.github.com>

* Shorten call-to-action

* Update what's new for v4.3.1

* Update to GOV.UK Frontend v4.3.1

* Add in link and download to Mural file

Interaction Designer, Clare Brown (clare.brown@digital.hmrc.gov.uk) has created an editable Mural template of GOV.UK Design System Flow Diagram for all designers to use.

This will help designers map out their service and/or user journeys at a high-level. The template is based off designs that were created in Miro and hosted on GitHub which can be found here: https://github.com/paulmsmith/govuk-designsystem-flow-diagram-miro.

File needs to be hosted for download, uploaded and have instructions on how people can use the file.

* Remove unused tables option

The `tables` option was removed in Marked v0.7.0 [1] so this doesn’t do anything any more.

[1]: https://github.com/markedjs/marked/releases/tag/v0.7.0

* Remove unused gfm: true from marked config

The documentation for the `pedantic` option [1] says:

> If true, conform to the original markdown.pl as much as possible. Don't fix original markdown bugs or behavior. Turns off and overrides gfm.

Verified that this option is doing nothing by comparing the build output with and without this setting, and there is no difference (other than the obfuscated email address on the community page, which is non-deterministic and so changes with every build):

```
$ diff -r deploy/public-marked-main deploy/public
diff -r deploy/public-marked-main/community/design-system-working-group/index.html deploy/public/community/design-system-working-group/index.html
1045c1045
< <p>In the meantime, if you are interested in joining the working group email the Design System team at <a href="mailto:&#103;&alphagov#111;&#x76;&alphagov#117;&#x6b;&#x2d;&#x64;&#101;&alphagov#115;&#105;&#x67;&alphagov#110;&#x2d;&alphagov#115;&#x79;&alphagov#115;&alphagov#116;&#101;&#x6d;&#x2d;&alphagov#115;&#x75;&#x70;&#x70;&#x6f;&alphagov#114;&#x74;&#x40;&#100;&#x69;&#x67;&#105;&alphagov#116;&#97;&#x6c;&#46;&#99;&#x61;&#x62;&#105;&alphagov#110;&#101;&#x74;&#45;&#x6f;&#x66;&#x66;&#105;&#99;&#x65;&#46;&#103;&#x6f;&#x76;&#x2e;&alphagov#117;&#x6b;">&#103;&alphagov#111;&#x76;&alphagov#117;&#x6b;&#x2d;&#x64;&#101;&alphagov#115;&#105;&#x67;&alphagov#110;&#x2d;&alphagov#115;&#x79;&alphagov#115;&alphagov#116;&#101;&#x6d;&#x2d;&alphagov#115;&#x75;&#x70;&#x70;&#x6f;&alphagov#114;&#x74;&#x40;&#100;&#x69;&#x67;&#105;&alphagov#116;&#97;&#x6c;&#46;&#99;&#x61;&#x62;&#105;&alphagov#110;&#101;&#x74;&#45;&#x6f;&#x66;&#x66;&#105;&#99;&#x65;&#46;&#103;&#x6f;&#x76;&#x2e;&alphagov#117;&#x6b;</a>.</p>
---
> <p>In the meantime, if you are interested in joining the working group email the Design System team at <a href="mailto:&#x67;&alphagov#111;&alphagov#118;&alphagov#117;&#x6b;&#x2d;&#100;&#101;&#x73;&#105;&#103;&#x6e;&#45;&alphagov#115;&#x79;&#x73;&alphagov#116;&#101;&alphagov#109;&#x2d;&alphagov#115;&#x75;&alphagov#112;&alphagov#112;&#x6f;&alphagov#114;&alphagov#116;&#x40;&#100;&#105;&#x67;&#105;&alphagov#116;&#97;&#x6c;&#x2e;&#x63;&#97;&#x62;&#x69;&alphagov#110;&#101;&alphagov#116;&#x2d;&#x6f;&#102;&#102;&#x69;&#99;&#x65;&#46;&#103;&alphagov#111;&alphagov#118;&#x2e;&alphagov#117;&alphagov#107;">&#x67;&alphagov#111;&alphagov#118;&alphagov#117;&#x6b;&#x2d;&#100;&#101;&#x73;&#105;&#103;&#x6e;&#45;&alphagov#115;&#x79;&#x73;&alphagov#116;&#101;&alphagov#109;&#x2d;&alphagov#115;&#x75;&alphagov#112;&alphagov#112;&#x6f;&alphagov#114;&alphagov#116;&#x40;&#100;&#105;&#x67;&#105;&alphagov#116;&#97;&#x6c;&#x2e;&#x63;&#97;&#x62;&#x69;&alphagov#110;&#101;&alphagov#116;&#x2d;&#x6f;&#102;&#102;&#x69;&#99;&#x65;&#46;&#103;&alphagov#111;&alphagov#118;&#x2e;&alphagov#117;&alphagov#107;</a>.</p>
```

[1]: https://marked.js.org/using_advanced#options

* Add border to 'preview' / 'archive' banner

We moved the banner when refactoring the nav in 2b6a27b.

Now that the banner is not between the header and the navigation, it makes sense to add the bottom border back to separate the banner from the page content.

Keep the `--no-border` modifier which we use on the homepage when the banner is followed immediately by the masthead.

* Remove redundant app-pane wrapper

When the Design System had an independently scrollable nav and body regions, we used a 'app-pane' flexbox wrapper around the whole site so that the header could be fixed in place above both scrollable regions.

When we simplified the layout and lost the indepedantly scrollable regions we didn't tidy up the wrapper, which isn't really doing anything any more (other than the flex layout causing weird issues like the banner width issue seen in 6c83fde).

Remove the unneccesary wrapper and its classes.

* Remove custom phase banner styles

The `width: 100%` is no longer needed that we're not in a flex context and can be removed.

As it turns out, the margins are being overridden by the `app-width-container` class anyway – and if they weren't being overriden, they'd mean that the phase banner would be flush up against the viewport edges on mobile, which isn't what we want.

* Remove redundant app-pane__content wrappers

The `app-pane__content` class isn't doing anything useful in these layouts and can be removed.

* Split 'category nav' out from app pane CSS

This doesn't really have anything to do with the rest of the 'split pane' CSS, so split it out into its own 'component'.

Whilst we're at it, give it a better name and some context to help explain what it's for and why we need it.

* Rename app-pane classes

* Fix implementation for non-flexbox browsers

* Rename 'pane' to 'split-pane'

* Remove unused CSS

* Stop recommending pattern=[0-9]* on number inputs

We previously recommended using `pattern="[0-9]*"` on number inputs to prompt iOS to display the numeric keypad.

This has been unnecessary since Safari 12.2, when support for the standardised `inputmode` attribute was added to Safari.

We stopped using the `pattern` attribute on the date input component [1] in v4.1.0 (released in May 2022) as the proportion of GOV.UK visitors using versions of iOS 12.x and below had fallen to 0.08% of total traffic.

[1]: alphagov/govuk-frontend#2599

Co-authored-by: Calvin Lau <77630796+calvin-lau-sig7@users.noreply.github.com>

* Remove pattern attribute from examples

See previous commit for details.

* Remove use of govuk-exports

We need to use govuk-exports in GOV.UK Frontend as files can be imported multiple files but we only ever want the CSS to be outputted once.

As we're in control of how and where Sass in the Design System is imported, we don't need to worry about this and don't need to use the exports function.

* Remove Sass and class that don't do anything

* Fix more Sass that doesn't do anything

The govuk-visually-hidden has `position: absolute` marked as `!important` and therefore it overrides this, so it can't be doing anything.

* Remove unused app-phase-banner class

* Update package-lock.json

* Remove new GOV.UK pages

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: christopherthomasdesign <chris.ballantinethomas@digital.cabinet-office.gov.uk>
Co-authored-by: Calvin Lau <77630796+calvin-lau-sig7@users.noreply.github.com>
Co-authored-by: Ciandelle Scollan <ciandelle.scollan@digital.cabinet-office.gov.uk>
Co-authored-by: Ciandelle Scollan <100923629+Ciandelle@users.noreply.github.com>
Co-authored-by: domoscargin <brett.kyle@digital.cabinet-office.gov.uk>
Co-authored-by: Vanita Barrett <vanita.barrett@digital.cabinet-office.gov.uk>
Co-authored-by: Frankie Roberto <frankie@frankieroberto.com>
Co-authored-by: ameliaphil <104211564+ameliaphil@users.noreply.github.com>
Co-authored-by: EoinShaughnessy <eoin.shaughnessy@digital.cabinet-office.gov.uk>
Co-authored-by: EoinShaughnessy <72507742+EoinShaughnessy@users.noreply.github.com>
Co-authored-by: Colin Oakley <colin.oakley@engineering.digital.dwp.gov.uk>
Co-authored-by: Ruth Hammond <91464546+ruthhammond@users.noreply.github.com>
Co-authored-by: Kimberly Grey <kimberly.grey@digital.cabinet-office.gov.uk>
Co-authored-by: Colin <colin@htmlandbacon.com>
Co-authored-by: Oliver Byford <oliver.byford@digital.cabinet-office.gov.uk>
Co-authored-by: StephenGill <stephen.gill@digital.cabinet-office.gov.uk>
Co-authored-by: Laurence de Bruxelles <laurence.debruxelles@digital.cabinet-office.gov.uk>
Co-authored-by: CAAshworth <claire.ashworth@digital.cabinet-office.gov.uk>
Co-authored-by: owenatgov <owen.jones@digital.cabinet-office.gov.uk>
Co-authored-by: Robert Deniszczyc <72561986+robertdeniszczyc2@users.noreply.github.com>
Co-authored-by: NoraGDS <57447099+NoraGDS@users.noreply.github.com>
Co-authored-by: Andy Mantell <134642+andymantell@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Thomas Leese <thomas@leese.io>
Co-authored-by: Nick Colley <2445413+nickcolley@users.noreply.github.com>
Co-authored-by: Ed Horsford <mail@edwardhorsford.com>
Co-authored-by: Peter Yates <peter.yates@graphia.co.uk>
Co-authored-by: Owen Jones <64783893+owenatgov@users.noreply.github.com>
Co-authored-by: Nick Colley <nick.colley@hippodigital.co.uk>
Co-authored-by: Clare Brown <107033520+clare-brown@users.noreply.github.com>
@NikhilNanjappa
Copy link

NikhilNanjappa commented Oct 5, 2022

Hi guys

Sorry I'm not following what we need to do to get this working. We recently moved from 4.1.0 to 4.3.1 and we have never set the assetUrl or opengraphImageUrl.

If our app is hosted on different domains - https://my-app.dev.dwpcloud.uk, https://my-app.test.dwpcloud.uk, https://my-app.stag.dwpcloud.uk etc, are we supposed to set absolute paths of these domain names dynamically like so ?

{% set assetPath = "https://my-app.dev.dwpcloud.uk/govuk/frontend/assets/images/govuk-opengraph-image.png" %}

@36degrees
Copy link
Contributor Author

@NikhilNanjappa

Most assets are intended to be loaded using relative paths, which means they use the assetPath variable which I think in your case should be /govuk/frontend/assets/.

However, the Open Graph images are different – because they're loaded by third parties they need to be absolute URLs. So, to get the Open Graph images to work, as well as the assetPath Nunjucks variable you should also set assetUrl to the absolute URL for the assets folder, like this:

{% set assetPath = '/govuk/frontend/assets/' %}
{% set assetUrl = "https://my-app.dev.dwpcloud.uk/govuk/frontend/assets" %}

Or, if you like:

{% set assetPath = '/govuk/frontend/assets/' %}
{% set assetUrl = "https://my-app.dev.dwpcloud.uk" + assetPath %}

As you've said, you'll need to be set assetUrl dynamically based on the environment.

(There is also the opengraphImageUrl variable, which you would use if your Open Graph image was somewhere else outside of the assets folder. If it helps understand how this works, you can see how they are used in the code here.)

@NikhilNanjappa
Copy link

Thanks @36degrees , thats what I had concerns regarding actually. Specifically getting the current domain value

{% set assetUrl = "https://my-app.dev.dwpcloud.uk" ...

I just find it a little weird as I would need to store that value dynamically in res.locals or similar and then use that in the nunjucks file.

@36degrees
Copy link
Contributor Author

I just find it a little weird as I would need to store that value dynamically in res.locals or similar and then use that in the nunjucks file.

How would you expect it to work?

@NikhilNanjappa
Copy link

NikhilNanjappa commented Oct 6, 2022

@36degrees probably use the relative path /govuk/frontend/assets/ by default and override it if user has provided assetUrl or opengraphImageUrl. Basically, if user has provided, use that or fallback to what you had pre 4.3.0 (/govuk/frontend/assets/)

This way projects aren't forced to now provide assetUrl or opengraphImageUrl with a minor upgrade.

@36degrees
Copy link
Contributor Author

36degrees commented Oct 6, 2022

Relative Open Graph image URLs aren't valid according to the spec. We felt that the previous implementation was effectively a bug as we should only output an Open Graph meta tag if we believe it will be valid, and it can't be valid unless we know the full URL.

Our thinking around the impact of this change is documented in the original PR.

I appreciate the feedback about this change being rolled out in a minor release, and recognise we could have maintained the existing behaviour until v5.0. That was a judgement call the team made at the time, and we're not always going to get everything 100% right.

It's worth noting that if you don't expect users to be sharing links to your service on social media, you probably don't need the OpenGraph image.

@NikhilNanjappa
Copy link

Thanks @36degrees I see, I completely understand.

You're right, we are not expecting users to share the links anyways so its no biggie :)

querkmachine pushed a commit that referenced this pull request Oct 17, 2022
#2673 was merged *after* v4.2.0 was released and remains an unreleased feature. There was no conflict in 536edad as the surrounding lines had not changed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow Open Graph image URL to be customised
4 participants