`EmbedCard` improvements #4503

patrickpatrickpatrick · 2025-02-24T17:16:53Z

What

Addresses comments left on the previous PR.

This includes:

Check if IntersectionObserver supported by browser before running code that adds IntersectionObservers
Move EmbedCard initialisation to a separate function, add extra functionality to cookie accepting/rejecting that would lend itself well to adding external content in the future
Change alt of the EmbedCard placeholder thumbnail
Use replaceWith in EmbedCard initialisation, remove div placeholder from template
Rewrite confusing .app-campaign-cookie-banner .govuk-grid-column-two-thirds selector to .app-campaign-cookie-banner div
Implement a specific page for testing EmbedCard and cookies
Fix issue where placeholder link height would change if no image
Various markup improvements
Remove link to YouTube thumbnail API
Use ytID instead of extracting the ID of YouTube video from href of link

Why

So that the EmbedCard component can be used to enhance the site and we can in the future add other external embeddable functionality (such as Soundcloud, Vimeo, etc).

If `IntersectionObserver` is not supported by the browser (it's not present on `window`) then don't run the `IntersectionObserver` code in `application.mjs` to enable lazy-load of the Youtube embeds. Instead, the fallback (linking to the Youtube video) will be used.

netlify · 2025-02-24T17:18:25Z

✅ You can preview this change here:

Name	Link
🔨 Latest commit	`8a93428`
🔍 Latest deploy log	https://app.netlify.com/sites/govuk-design-system-preview/deploys/67c1dd20219ba10008b249a9
😎 Deploy Preview	https://deploy-preview-4503--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.

romaricpascal

Cheers for making these updates, I think it's getting there, just noticed a couple of little things as I went through 🙌🏻

romaricpascal · 2025-02-27T10:14:16Z

src/javascripts/components/cookie-banner.mjs

@@ -113,6 +113,9 @@ class CookieBanner extends ConfigurableComponent {
    // Hide banner and show confirmation message
    this.$cookieMessage.setAttribute('hidden', 'true')
    this.revealConfirmationMessage(this.$cookieConfirmationAccept)
+
+    // Dispatch custom event to notify listening components of change
+    document.dispatchEvent(new CustomEvent('cookieChange'))


🙌🏻 I like that as an option to let the cookie banner make other parts of the page aware cookies were accepted.

romaricpascal · 2025-02-27T10:17:01Z

src/javascripts/components/embed-initialisation.mjs

+  if (userConsent && userConsent.campaign) {
+    addEmbedCardObservers($embedCards)
+  } else {
+    document.addEventListener('cookieChange', function () {


isssue This will also trigger when the cookies are rejected, as the event is also fired in rejectCookies. Given we're now only setting up observers if cookies are accepted, I propose:

not dispatching an event in rejectCookies as we have no code responding to that

renaming the event in acceptCookies to cookieAccepted (in case we need a cookieRejected in the future)

src/javascripts/application.mjs

src/javascripts/components/embed-card.mjs

views/partials/_embed-card.njk

romaricpascal · 2025-02-27T11:28:10Z

src/stylesheets/main.scss

@@ -440,6 +440,6 @@ pre .language-plaintext {
  }
 }

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


issue div is quite generic and we risk side effects from this selector. I've asked on Slack for other ideas to avoid contradicting two-thirds in the name with 100% in the properties.

romaricpascal · 2025-02-27T11:31:31Z

src/embed-test.md

@@ -47,11 +41,6 @@ ignoreInSitemap: true
 {% call embedCard({
  ytId: "x91MPoITQ3I",
  transcriptHref: "https://www.google.com",
-  authorImgSrc: "/images/dsd24-day1-speakers1.png",


issue Could we have a second embed card that shows the author image, and possibly a test using it if there's none already?

Added another template, seems difficult to determine intersection in puppeteer.

Ah, that's a shame, that would have allowed us to automatically test the IntersectionObserver worked as well 😔 Is it a lack of native methods from Puppeteer for triggering the intersection (if it is we can maybe try with native DOM methods like scrollIntoView) or just more general issues with IntersectionObserver?

src/stylesheets/components/_embed-card.scss

- EmbedCard initialisation moved to `embed-initialisation.mjs` - Fire `cookieChange` event when user accepts/rejects cookies - Add `IntersectionObserver` if user has accepted cookies already or if user accepts cookies on current page

Instead of creating the new element, appending the new element and then deleting the placeholder element. Now we replace the placeholder with the new element instead of deleting the placeholder explicitly.

Remove confusing `.app-campaign-cookie-banner .govuk-grid-column-two-thirds` selector and replace with `div`.

Remove previous page for testing that was a draft of a Design System Day promotion page. Add new `embed-test` page specifically for testing `EmbedCard` and cookies.

If thumbnail image fails to load in the EmbedCard placeholder, it will no longer collapse the size of the placeholder link.

YouTube Thumbnail API sets cookies so use a thumbnail image from the application itself instead.

romaricpascal

Cheers for the updates. Noticed two extra things that I didn't spot in the previous review, sorry 😔

romaricpascal · 2025-02-28T16:24:01Z

src/javascripts/components/cookie-banner.mjs

@@ -113,6 +113,9 @@ class CookieBanner extends ConfigurableComponent {
    // Hide banner and show confirmation message
    this.$cookieMessage.setAttribute('hidden', 'true')
    this.revealConfirmationMessage(this.$cookieConfirmationAccept)
+
+    // Dispatch custom event to notify listening components of change
+    document.dispatchEvent(new CustomEvent('cookieAccepted'))


issue Something I didn't think about with the first comment, sorry, is that there might be two cookie banners on the page (the top one and the one for the videos). We need to distinguish which cookies have been accepted in that event, as videos can only be enabled if the campaign cookie was accepted.

romaricpascal · 2025-02-28T16:30:49Z

src/javascripts/application.mjs

-    subtree: true
-  })
-}
+embedCardsInitialisation()


nitpick For consistency with createAll we should probably write this as initialiseEmbedCards (active verb rather than a noun).

patrickpatrickpatrick changed the title ~~yt embed improve~~ EmbedCard improvements Feb 24, 2025

patrickpatrickpatrick force-pushed the yt-embed-improve branch 3 times, most recently from 5a892a3 to c711114 Compare February 25, 2025 13:59

patrickpatrickpatrick marked this pull request as ready for review February 25, 2025 14:33

patrickpatrickpatrick force-pushed the yt-embed-improve branch from c711114 to 1c41f10 Compare February 25, 2025 14:35

patrickpatrickpatrick changed the title ~~EmbedCard improvements~~ EmbedCard improvements Feb 25, 2025

romaricpascal reviewed Feb 27, 2025

View reviewed changes

patrickpatrickpatrick force-pushed the yt-embed-improve branch from 1c41f10 to 1c3e211 Compare February 28, 2025 12:02

patrickpatrickpatrick added 2 commits February 28, 2025 12:03

Move EmbedCard initialisation to seperate function

a3ca4b2

- EmbedCard initialisation moved to `embed-initialisation.mjs` - Fire `cookieChange` event when user accepts/rejects cookies - Add `IntersectionObserver` if user has accepted cookies already or if user accepts cookies on current page

Change alt of Embed Card thumbnail placeholder

22186c5

patrickpatrickpatrick force-pushed the yt-embed-improve branch 2 times, most recently from 73f9e07 to 127c76c Compare February 28, 2025 15:06

patrickpatrickpatrick added 7 commits February 28, 2025 15:42

Replace EmbedCard placeholder element

a7fe822

Instead of creating the new element, appending the new element and then deleting the placeholder element. Now we replace the placeholder with the new element instead of deleting the placeholder explicitly.

Replace govuk-grid-column-two-thirds with div

10e59a1

Remove confusing `.app-campaign-cookie-banner .govuk-grid-column-two-thirds` selector and replace with `div`.

Implement embed-test page

3b612a9

Remove previous page for testing that was a draft of a Design System Day promotion page. Add new `embed-test` page specifically for testing `EmbedCard` and cookies.

Ensure EmbedCard placeholder height consistent

0bd84db

If thumbnail image fails to load in the EmbedCard placeholder, it will no longer collapse the size of the placeholder link.

Remove unneeded markup in _embed-card.njk

eb6d446

Remove link to Youtube Thumbnail API

81ce6a2

YouTube Thumbnail API sets cookies so use a thumbnail image from the application itself instead.

Remove regex from EmbedCard, use Youtube ID

3a71e6a

patrickpatrickpatrick force-pushed the yt-embed-improve branch from 127c76c to 12d68c3 Compare February 28, 2025 15:55

Test for EmbedCard with and without author image

8a93428

patrickpatrickpatrick force-pushed the yt-embed-improve branch from 12d68c3 to 8a93428 Compare February 28, 2025 15:58

romaricpascal reviewed Feb 28, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`EmbedCard` improvements #4503

`EmbedCard` improvements #4503

patrickpatrickpatrick commented Feb 24, 2025 •

edited

Loading

netlify bot commented Feb 24, 2025 •

edited

Loading

romaricpascal left a comment

romaricpascal Feb 27, 2025

romaricpascal Feb 27, 2025

romaricpascal Feb 27, 2025

romaricpascal Feb 27, 2025

patrickpatrickpatrick Feb 28, 2025

romaricpascal Feb 28, 2025

romaricpascal left a comment

romaricpascal Feb 28, 2025

romaricpascal Feb 28, 2025

EmbedCard improvements #4503

Are you sure you want to change the base?

EmbedCard improvements #4503

Conversation

patrickpatrickpatrick commented Feb 24, 2025 • edited Loading

What

Why

netlify bot commented Feb 24, 2025 • edited Loading

✅ You can preview this change here:

romaricpascal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romaricpascal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

`EmbedCard` improvements #4503

`EmbedCard` improvements #4503

patrickpatrickpatrick commented Feb 24, 2025 •

edited

Loading

netlify bot commented Feb 24, 2025 •

edited

Loading