Add Youtube Embed Support to the ImageCard Component

[patrickpatrickpatrick]

What

New variant of image card that supports embedding a Youtube video.

More Details

ImageCardHelper modified to accept a youtube_video_id and a set of new functions for generating a Youtube embed and a placeholder link using a thumbnail of the Youtube embed.

YoutubeLinkEnhancement can accept an override class if you want an alternative class to be appended to the youtube embed that isn't gem-c-govspeak__youtube-video.

Why

As part of the new design for No. 10 organisation page we need to be able to show promotional content with embeddable videos. Promotional content currently uses the image card component, so this PR adds support for embedding specifically Youtube videos into the image card component. Youtube embed functionality already exists in govspeak, therefore we are using that to generate the embed as it already has the functionality we need.

Screenshot

[chao-xian]

@patrickpatrickpatrick lovely work. Small thing: I'd split this commit 1d83fb6 first into one where you refactor to split the partial, and then add in the youtube bits

[andysellick]

Obviously still draft so I won't do a full review but one concern I'd like to raise. At the moment the component auditing relies on the fact that component files use the same name for themselves (i.e. the image card component has an image card template, image card scss file, etc.) in order to report accurate information, but this change looks like it might break that convention. Would it be possible to have the filenames for the various parts of this match the component names in order to preserve how the auditing works? Happy to discuss if this isn't clear.

[patrickpatrickpatrick]

Obviously still draft so I won't do a full review but one concern I'd like to raise. At the moment the component auditing relies on the fact that component files use the same name for themselves (i.e. the image card component has an image card template, image card scss file, etc.) in order to report accurate information, but this change looks like it might break that convention. Would it be possible to have the filenames for the various parts of this match the component names in order to preserve how the auditing works? Happy to discuss if this isn't clear.

I believe this should be the case in the way I've implemented it. Youtube card has a youtube_card.html.erb, yotube_card.js, youtube_card.scss; Image card has a image_card.html.erb, image_card.scss and media card has a media_card.html.erb and media_card.scss.

[andysellick]

Thanks for the offline chat @patrickpatrickpatrick I think I understand this better now. As discussed, I've plugged this branch into an application to test how the auditing responds.

The auditing isn't detecting that the image card and youtube card components contain media card, and therefore doesn't correctly report problems to do with assets (e.g. not including the media card scss). It does allow for components that contain other components, but in this case the media card is considered a partial and not a top level component - it's importing /components/media_card/media_card (the pattern for a partial) and not /components/media_card (the pattern for a separate component). Unfortunately this distinction has to be made otherwise the auditing has no way of determining the difference between a component (where it should warn about missing assets) and a partial (where it shouldn't warn about missing assets) and of course we don't want to make the media card a separate component.

There's a similar problem with the 'suggested imports for this application' functionality (which appears on the front page of the component guide inside an application).

Not sure what to do about this - we don't want to make the media card its own component, but image card and youtube card rely on the assets from media card. Some options:

• this could be solved both by an adjustment in this PR and a change to the auditing but I'd have to look into how it works, can't guarantee anything
• could the image card and youtube card import the relevant Sass directly? I'm not sure, there might be a problem of duplication
• totally crazy idea: what if there was just one component, but depending on what you pass it, it works either as the image card or the youtube card? You'd keep the helpers almost as they are, but the component would figure out which was being called then include the relevant partial (image card or youtube card). The Sass could be combined again and the whole problem could be avoided.

(I think this will be ultimately solved by the work being done where components include their own assets, but that's a little while off yet unfortunately)

[patrickpatrickpatrick]

Thanks for the offline chat @patrickpatrickpatrick I think I understand this better now. As discussed, I've plugged this branch into an application to test how the auditing responds.

The auditing isn't detecting that the image card and youtube card components contain media card, and therefore doesn't correctly report problems to do with assets (e.g. not including the media card scss). It does allow for components that contain other components, but in this case the media card is considered a partial and not a top level component - it's importing /components/media_card/media_card (the pattern for a partial) and not /components/media_card (the pattern for a separate component). Unfortunately this distinction has to be made otherwise the auditing has no way of determining the difference between a component (where it should warn about missing assets) and a partial (where it shouldn't warn about missing assets) and of course we don't want to make the media card a separate component.

There's a similar problem with the 'suggested imports for this application' functionality (which appears on the front page of the component guide inside an application).

Not sure what to do about this - we don't want to make the media card its own component, but image card and youtube card rely on the assets from media card. Some options:

• this could be solved both by an adjustment in this PR and a change to the auditing but I'd have to look into how it works, can't guarantee anything
• could the image card and youtube card import the relevant Sass directly? I'm not sure, there might be a problem of duplication
• totally crazy idea: what if there was just one component, but depending on what you pass it, it works either as the image card or the youtube card? You'd keep the helpers almost as they are, but the component would figure out which was being called then include the relevant partial (image card or youtube card). The Sass could be combined again and the whole problem could be avoided.

(I think this will be ultimately solved by the work being done where components include their own assets, but that's a little while off yet unfortunately)

I think of those options, the later option might be the best solution here. The reason I implemented this way was to reduce the amount of logic required by just being to call the specific card that we want to use directly, but this way I could I always put that logic in the helper. Will get to implementing it!

[andysellick]

Think this all looks good now, good job. One conflict to fix and might be worth squashing some of the commits together, but otherwise good 👍