Add Embed Card Component and Update Cookies by patrickpatrickpatrick · Pull Request #3963 · alphagov/govuk-design-system

patrickpatrickpatrick · 2024-07-29T10:00:17Z

NB: Content will come in a later PR. Felt that the work in PR should be separated as it was getting quite large.

What

Adds new Embed Card Component
Updates Cookie functionality
Adds relevant tests
New page as an example of functionality for review

Why

We want to add the ability to embed YouTube videos to the Design System website. This requires new preference cookies needing to be set because embeddable iframes from external sites can/do set cookies themselves.

Addresses #3931

netlify · 2024-07-29T10:02:19Z

✅ You can preview this change here:

Name	Link
🔨 Latest commit	`781b769`
🔍 Latest deploy log	https://app.netlify.com/sites/govuk-design-system-preview/deploys/66c5e18f1e8f0300087d5c64
😎 Deploy Preview	https://deploy-preview-3963--govuk-design-system-preview.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

patrickpatrickpatrick · 2024-08-05T15:12:46Z

When it's approved, the page will be set to DRAFT.

owenatgov

Couple of comments. Overall looking good!

owenatgov · 2024-08-07T14:46:33Z

src/community/design-system-day-2024-day-1/index.md

+
+{% from "_cookie-banner.njk" import cookieBanner %}
+
+<p class="govuk-!-font-size-24">


You could use govuk-body-l here. Functionally they're the same but for neatness we should probably be using native styles rather than overrides where we can.

Suggested change

<p class="govuk-!-font-size-24">

<p class="govuk-body-l">

src/community/design-system-day-2024-day-1/index.md

owenatgov · 2024-08-07T15:01:50Z

src/stylesheets/components/_embed-card.scss

+  width: 100%;
+  height: 100%;
+  background:
+    url("data:image/svg+xml;base64,PHN2ZyB4bWxucz0iaHR0cDovL3d3dy53My5vcmcvMjAwMC9zdmciIHdpZHRoPSI3OSIgaGVpZ2h0PSI1NSIgdmlld0JveD0iMCAwIDc5IDU1IiBmaWxsPSJub25lIj4KPHBhdGggZD0iTTM5Ljc1MDMgMEg0MC4xODYxQzQ0LjIxMDQgMC4wMTQ2ODEgNjQuNjAxNiAwLjE2MTQ5MiA3MC4wOTk2IDEuNjM5MzhDNzEuNzYxNyAyLjA5MDQyIDczLjI3NjMgMi45Njk1OSA3NC40OTIgNC4xODkwMkM3NS43MDc2IDUuNDA4NDYgNzYuNTgxOCA2LjkyNTQ1IDc3LjAyNzIgOC41ODg0Qzc3LjUyMTcgMTAuNDQ4IDc3Ljg2OTMgMTIuOTA5NSA3OC4xMDQzIDE1LjQ0OTNMNzguMTUzMyAxNS45NTgzTDc4LjI2MSAxNy4yMzA2TDc4LjMwMDEgMTcuNzM5NkM3OC42MTg0IDIyLjIxMjQgNzguNjU3NSAyNi40MDE0IDc4LjY2MjQgMjcuMzE2NVYyNy42ODM1Qzc4LjY1NzUgMjguNjMyOSA3OC42MTM1IDMzLjEwNTcgNzguMjYxIDM3Ljc2NDVMNzguMjIxOCAzOC4yNzgzTDc4LjE3NzcgMzguNzg3M0M3Ny45MzI5IDQxLjU4NjQgNzcuNTcwNiA0NC4zNjYgNzcuMDI3MiA0Ni40MTE2Qzc2LjU4MzIgNDguMDc1MiA3NS43MDk0IDQ5LjU5MjkgNzQuNDkzNiA1MC44MTI2QzczLjI3NzcgNTIuMDMyMiA3MS43NjI0IDUyLjkxMDkgNzAuMDk5NiA1My4zNjA2QzY0LjQyMDQgNTQuODg3NCA0Mi44MzQ3IDU0Ljk5NTEgMzkuODQzNCA1NUgzOS4xNDgxQzM3LjYzNTMgNTUgMzEuMzc4NSA1NC45NzA2IDI0LjgxOCA1NC43NDU1TDIzLjk4NTggNTQuNzE2MkwyMy41NTk4IDU0LjY5NjZMMjIuNzIyNiA1NC42NjIzTDIxLjg4NTQgNTQuNjI4MUMxNi40NTExIDU0LjM4ODMgMTEuMjc2MiA1NC4wMDE3IDguODkxOSA1My4zNTU3QzcuMjI5NzIgNTIuOTA2NSA1LjcxNDgyIDUyLjAyODUgNC40OTg5OSA1MC44MDk3QzMuMjgzMTUgNDkuNTkxIDIuNDA5MDcgNDguMDc0MyAxLjk2NDMgNDYuNDExNkMxLjQyMDg2IDQ0LjM3MDkgMS4wNTg1NyA0MS41ODY0IDAuODEzNzc2IDM4Ljc4NzNMMC43NzQ2MDkgMzguMjczNEwwLjczNTQ0MyAzNy43NjQ1QzAuNDkzODIyIDM0LjQ0ODUgMC4zNTk5MzIgMzEuMTI1NiAwLjMzMzk4NCAyNy44MDFMMC4zMzM5ODQgMjcuMTk5QzAuMzQzNzc2IDI2LjE0NjkgMC4zODI5NDMgMjIuNTEwOSAwLjY0NzMxOCAxOC40OTgxTDAuNjgxNTg5IDE3Ljk5NEwwLjY5NjI3NiAxNy43Mzk2TDAuNzM1NDQzIDE3LjIzMDZMMC44NDMxNTEgMTUuOTU4M0wwLjg5MjEwOSAxNS40NDkzQzEuMTI3MTEgMTIuOTA5NSAxLjQ3NDcxIDEwLjQ0MzEgMS45NjkxOSA4LjU4ODRDMi40MTMyMSA2LjkyNDc4IDMuMjg2OTcgNS40MDcwNyA0LjUwMjg1IDQuMTg3NDNDNS43MTg3NCAyLjk2Nzc5IDcuMjM0MDIgMi4wODkwNyA4Ljg5NjggMS42MzkzOEMxMS4yODExIDEuMDAzMiAxNi40NTYgMC42MTE3MDkgMjEuODkwMyAwLjM2NzAyNUwyMi43MjI2IDAuMzMyNzdMMjMuNTY0NyAwLjMwMzQwOUwyMy45ODU4IDAuMjg4NzI3TDI0LjgyMjkgMC4yNTQ0NzJDMjkuNDgyMyAwLjEwNDYwMSAzNC4xNDM3IDAuMDIxMzk5NCAzOC44MDU0IDAuMDA0ODkzOUgzOS43NTAzVjBaTTMxLjY2NzMgMTUuNzA4N1YzOS4yODY0TDUyLjAxOTMgMjcuNTAyNEwzMS42NjczIDE1LjcwODdaIiBmaWxsPSIjRUEzMzIzIi8+CjxwYXRoIGQ9Ik0zMS42NjggMTUuNzA4N1YzOS4yODY1TDUyLjAxOTkgMjcuNTAyNUwzMS42NjggMTUuNzA4N1oiIGZpbGw9IndoaXRlIi8+Cjwvc3ZnPg==")


Where is this coming from?

Oh, this is just me converting the YouTube logo to a base64 string. Can add it as an actual image if that's preferable.

Is there a benefit to either appraoch? The only downside I can see here is that it amkes the file slightly less legible. If we can add a comment explaining what it is and why that's good enough for me.

One thing to think about: inlined in the CSS, it adds Kb of downloads to each page, even those that don't show any video. If referenced as an external URL, browsers will only download it on pages showing videos 😊

I'll switch to using an asset :)

owenatgov · 2024-08-07T15:05:13Z

views/partials/_cookie-banner.njk

-  <p class="govuk-body">You’ve accepted analytics cookies. You can <a class="govuk-link" href="/cookies/">change your cookie settings</a> at any time.</p>
-{% endset %}
+  {%- set category -%}
+    {%- if not params.category -%}


How come we're checking for params negatively? IE: Can we change the if around so we're just checking for params.[param]?

views/partials/_cookie-banner.njk

Adds new category of cookies to cookie functions and the cookie page. Change default values of preference cookies to `null`. This means that we can now track if the user has been asked at all what their cookie preference is for different categories.

Change `_cookie-banner.njk` to a macro that take parameters including what category of cookie the banner is setting and the HTML that should be displayed for different states of the cookie banner. Allow `cookie-banner.mjs` to have a different cookie category than 'analytics' to be set (by using data varibles on the target module). Move the `GDS_CONSENT_COOKIE_VERSION` to `_generic.njk` so that this only gets set once at the top each page instead of multiple times by different cookie banner instances. Add new tests for multiple categories of cookie banners on the same page. Update existing tests.

Create the Design System Day 1 page, which will consist of embeds of talks of the speakers.

Update `application.mjs` to support multiple cookie banners on a page. Create new Embed Card component. If require cookie not set (or script fails to execute) placeholder image that links to YouTube video will be created. If cookie is set then placeholder will be replaced with a YT embed. Add `lazyEmbedObserver` in `application.mjs`. Embed Card component will only be initialized when the placeholder is in the viewport. This means for a page with lots of embeds, the page the performance hit will be limited if the user doesn't scroll the entire page. Add tests for new Embed Card component.

owenatgov

😎

romaricpascal

Apologies for the late review, it's a strong base to add embeds but I've found a couple of places we might improve things. These can be adressed in separate PRs now we've merged 😊

romaricpascal · 2024-08-22T08:49:49Z

__tests__/cookie-banner.test.js

+        })
+
+        await page.setCookie({ ...cookieParam, value })
+        await goTo(page, '/community/design-system-day-2024-day-1/')


suggestion Could we use a draft test page instead of a live content page everywhere we're running tests? This will reduce the risk of a content update screwing the tests if one day we archive or move the page for DS Day day 1.

romaricpascal · 2024-08-22T08:57:54Z

src/javascripts/application.mjs

+
+const $embedCards = document.querySelectorAll('[data-module="app-embed-card"]')
+
+const lazyEmbedObserver = new IntersectionObserver(function (


question Looks like there's a slim gap of lack of support for IntersectionObserver in Safari where this code will error. Thanfully this is the last thing being initialised, so other components won't be affected, but if an error occurs due to lack of IntersectionObserver, will people still be able to navigate to the video via the link already in the page?

suggestion For tidiness, it would be good to protect the component's initialisation with an if (IntersectionObserver in window) or something similar 😊

romaricpascal · 2024-08-22T09:01:32Z

views/partials/_embed-card.njk

+      {{- caller() if caller else params.content | safe -}}
+      <a class="app-embed-card__placeholder" href="https://www.youtube.com/watch?v={{ params.ytId }}">
+        <div class="app-embed-card__placeholder-thumb-container">
+          <img class="app-image--no-border app-embed-card__placeholder-thumb-image" alt="placeholder for {{ params.title }} youtube embed" src="https://img.youtube.com/vi/{{ params.ytId }}/hqdefault.jpg">


nitpick Worth checking with @selfthinker, but I don't think the alternative text brings any information to a person using assistive technology and will just lengthen the name of the link to "placeholder for <VIDEO_TITLE> youtube embed <VIDEO_TITLE>".

If we set the alt text to {{ params.title}} this will:

provide a fallback in case the placeholder image doesn't load for whatever reason

reduce the markup, as the next span will be redundant for labelling the link and we can get rid of it

question A couple of questions about the URL we use for that placeholder:

is it from an official API from YouTube or discovered from how their embed works? how likely is it to change in the future?

it effectively embeds content from another domain into our site, are we sure it doesn't set cookies prior to users having given their consent (and won't do in the future)?

All in all, it might be safer to have the placeholder images in our repo (we're the ones making the videos anyway so we have the images, in any case).

romaricpascal · 2024-08-22T09:29:43Z

src/javascripts/application.mjs

+        lazyEmbedObserver.unobserve(lazyEmbed)
+        lazyEmbedObserver.observe(lazyEmbed)


question I'm not following what this is doing exactly, what's stopping observing and observing again doing?

romaricpascal · 2024-08-22T09:46:04Z

src/javascripts/application.mjs

+  }
+
+  const observer = new MutationObserver(callback)
+  observer.observe(campaignCookieBanner, {


question Do we need to unobserve the mutations at some point? Thinking the CookieBanner may have two mutations happening: the click for 'Accept/Refuse', then the click to dismiss the banner?

romaricpascal · 2024-08-22T10:11:33Z

src/stylesheets/main.scss

  }
 }
+
+.app-campaign-cookie-banner .govuk-grid-column-two-thirds {


nitpick This makes the semantic of govuk-grid-column-two-thirds surprising. Is there a way we could use either:

a width override class

a different class to hook the styles to than .govuk-grid-column-two-thirds

romaricpascal · 2024-08-22T10:19:13Z

src/javascripts/components/embed-card.mjs

+      const ytId = ytHref.match(
+        /(youtu\.be\/|youtube\.com\/(watch\?(.*&)?v=|(embed|v)\/))([^?&"'>]+)/
+      )[5]
+
+      const iframe = this.createIframe(ytId, title)


suggestion Given the Nunjucks component knows the ID, passing it through data-attribute would avoid us maintaining a RegExp to extract it from a URL we've built ourselves.

romaricpascal · 2024-08-22T10:55:40Z

src/stylesheets/components/_embed-card.scss

@@ -0,0 +1,17 @@
+.app-embed-card__placeholder-thumb-container {


issue If the image does not load, the thumbnail container becomes very short. Best to give it a set aspect ratio (we'll still need the padding hack to do it).

The aspect ratio would also be beneficial to the iframe while it's loading, to avoid layout shifts.

romaricpascal · 2024-08-22T10:57:37Z

views/partials/_embed-card.njk

+    <div class="govuk-grid-column-two-thirds-from-desktop">
+      {{- caller() if caller else params.content | safe -}}
+      <a class="app-embed-card__placeholder" href="https://www.youtube.com/watch?v={{ params.ytId }}">
+        <div class="app-embed-card__placeholder-thumb-container">


question Could the <a> serve as thumb container if it was display: block and lighten the HTML a little?

romaricpascal · 2024-08-22T10:59:58Z

views/partials/_embed-card.njk

@@ -0,0 +1,22 @@
+{% macro embedCard(params) %}
+  <div class="govuk-grid-row" data-module="app-embed-card">


suggestion Given the role of the JavaScript is to swap the link for an iframe, having it set on the <a> element would help narrow down which bits of the markup it would affect.

patrickpatrickpatrick force-pushed the design-system-day-yt branch from eddda80 to 8aa19bc Compare July 31, 2024 10:12

patrickpatrickpatrick changed the title ~~design system day yt~~ Add Embed Card Component Jul 31, 2024

patrickpatrickpatrick changed the title ~~Add Embed Card Component~~ Add Embed Card Component and Update Cookies Jul 31, 2024

patrickpatrickpatrick force-pushed the design-system-day-yt branch 2 times, most recently from d69467a to 3676f37 Compare July 31, 2024 10:31

patrickpatrickpatrick marked this pull request as ready for review July 31, 2024 10:39

patrickpatrickpatrick linked an issue Jul 31, 2024 that may be closed by this pull request

Investigate youtube video embedding for the DS website #3931

Closed

6 tasks

patrickpatrickpatrick marked this pull request as draft August 1, 2024 11:07

patrickpatrickpatrick force-pushed the design-system-day-yt branch 6 times, most recently from 797f9a8 to 32a9744 Compare August 5, 2024 14:06

patrickpatrickpatrick marked this pull request as ready for review August 5, 2024 15:05

patrickpatrickpatrick force-pushed the design-system-day-yt branch from 32a9744 to 005c467 Compare August 5, 2024 15:11

patrickpatrickpatrick force-pushed the design-system-day-yt branch from 005c467 to 8fa1fa9 Compare August 5, 2024 15:21

patrickpatrickpatrick requested a review from a team August 7, 2024 10:46

owenatgov reviewed Aug 7, 2024

View reviewed changes

patrickpatrickpatrick force-pushed the design-system-day-yt branch 2 times, most recently from 7043be0 to f00aa8d Compare August 12, 2024 09:54

patrickpatrickpatrick requested review from owenatgov and romaricpascal August 20, 2024 13:08

patrickpatrickpatrick force-pushed the design-system-day-yt branch from f00aa8d to f9cb361 Compare August 21, 2024 12:22

patrickpatrickpatrick added 3 commits August 21, 2024 13:23

Add Campaign Cookies to Cookie Page

b1ec826

Adds new category of cookies to cookie functions and the cookie page. Change default values of preference cookies to `null`. This means that we can now track if the user has been asked at all what their cookie preference is for different categories.

Design System Day 1 page

d2d6ca4

Create the Design System Day 1 page, which will consist of embeds of talks of the speakers.

patrickpatrickpatrick force-pushed the design-system-day-yt branch from f9cb361 to 5399fe6 Compare August 21, 2024 12:29

patrickpatrickpatrick force-pushed the design-system-day-yt branch from 5399fe6 to 781b769 Compare August 21, 2024 12:46

owenatgov approved these changes Aug 21, 2024

View reviewed changes

patrickpatrickpatrick merged commit a88c287 into main Aug 21, 2024

patrickpatrickpatrick deleted the design-system-day-yt branch August 21, 2024 13:06

romaricpascal reviewed Aug 22, 2024

View reviewed changes

romaricpascal mentioned this pull request Sep 10, 2024

Improve implementation of Embed Card component #4122

Open

1 task

patrickpatrickpatrick mentioned this pull request Feb 24, 2025

EmbedCard improvements #4503

Open


		{% from "_cookie-banner.njk" import cookieBanner %}

		<p class="govuk-!-font-size-24">


		const $embedCards = document.querySelectorAll('[data-module="app-embed-card"]')

		const lazyEmbedObserver = new IntersectionObserver(function (

		lazyEmbedObserver.unobserve(lazyEmbed)
		lazyEmbedObserver.observe(lazyEmbed)

		@@ -0,0 +1,17 @@
		.app-embed-card__placeholder-thumb-container {

		@@ -0,0 +1,22 @@
		{% macro embedCard(params) %}
		<div class="govuk-grid-row" data-module="app-embed-card">

Conversation

patrickpatrickpatrick commented Jul 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Why

Uh oh!

netlify bot commented Jul 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ You can preview this change here:

Uh oh!

patrickpatrickpatrick commented Aug 5, 2024

Uh oh!

owenatgov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

owenatgov left a comment

Choose a reason for hiding this comment

Uh oh!

romaricpascal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

patrickpatrickpatrick commented Jul 29, 2024 •

edited

Loading

netlify bot commented Jul 29, 2024 •

edited

Loading