Feature: Responsive media component by peterschewe · Pull Request #45 · jungvonmatt/wekit

peterschewe · 2022-09-21T16:17:03Z

This PR merges all media components (c-image, c-video, c-media) into a new component c-responsive-media.

Todos

TBD

Image quality: Do we need type specific (JPEG, PNG, AVIF, WebP) quality settings? The Default for all is 80 I think. Here (https://www.industrialempathy.com/posts/avif-webp-quality-settings/) we find good presets for AVIF and WebP.
Core Utils: Are the adjustments to the core utils (see above) breaking changes?
Caption: Where do we like to use a caption? c-responsive-media vs. m-media
Video:
- Do we need a poster attribute at the video element? I'm currently using a transparent data image.
- We don't set a playsinline attribute in the video util yet. I'm not sure if it would cause problems on iOS.
- We already use ffmpeg in cssg. The step to automatically generate smaller videos and in other formats (e.g. webm) would be obvious. However, it would slow down the build time. But maybe it could run as a separate task – e.g. when creating new assets, trigger a webhook directly, which provides the new files via Netlify function and sends them back to Contentful.

New component: Responsive media

The component simplifies the handling of images and short loop videos, which can be used in the hero or media module, for example.

The name (c-responsive-media instead of c-media) is also a clearer distinction from the module m-media.

Features

Can display different media for each breakpoint, e.g. an image on mobile and a video on desktop.
Ratios and focus areas can be defined in Contentful. Images are cropped accordingly via Contentful's Image API. Video files remain in the original and are then masked in the frontend.
(wip) Video posters are generated automatically via cssg, incl. different sizes and types.

Contentful

Markup (simplified)

<!-- Example 1: Mobile image; desktop video; different ratios; image without lazy loading (for use above the fold) -->
<figure>
  <picture>
    <source srcset="poster-desktop.jpg" media="(min-width: 768px)" width="1920" height="1080">
    <source srcset="image-mobile.jpg" media="(max-width: 767px)" width="768" height="768">
    <img src="image-mobile.jpg" alt="Lorem ipsum" decoding="auto" fetchpriority="high">
  </picture>
  <video class="-md:u-hidden" poster="transparent data/base64 image">
    <source data-src="video-desktop.mp4" type="video/mp4">
  </video>
</figure> 

<!-- Example 2: Mobile video; desktop video; different sizes; image with lazy loading -->
<figure>
  <picture>
    <source srcset="poster-desktop.jpg" media="(min-width: 768px)" width="1920" height="1080">
    <source srcset="poster-mobile.jpg" media="(max-width: 767px)" width="768" height="432">
    <img src="poster-mobile.jpg" alt="Lorem ipsum" decoding="auto" loading="lazy">
  </picture>
  <video class="-md:u-hidden" poster="transparent data/base64 image">
    <source data-src="video-desktop.mp4" type="video/mp4">
  </video>
  <video class="md:u-hidden" poster="transparent data/base64 image">
    <source data-src="video-mobile.mp4" type="video/mp4">
  </video>
</figure> 

<!-- Example 3: Mobile image; desktop image; different ratios; image with lazy loading -->
<figure>
  <picture>
    <source srcset="image-desktop.jpg" media="(min-width: 768px)" width="1920" height="1080">
    <source srcset="image-mobile.jpg" media="(max-width: 767px)" width="768" height="768">
    <img src="image-mobile.jpg" alt="Lorem ipsum" decoding="auto" loading="lazy">
  </picture>
</figure>

…<img> This change is important if different ratios are defined via sources. The img itself must not have a default size.

… preloading

bezoerb

Looks good already :)
Nice work @peterschewe

bezoerb · 2022-09-21T22:14:52Z

hugo-modules/core/utils/asset/image/img.html

      {{- $width := .width -}}
      {{- $height := .height -}}
      {{/* Preload hint is added in sources.html */}}
-       <img src="{{- $src -}}" width="{{- $width -}}" height="{{- $height -}}" {{ $image_attributes }} alt="{{- $alt -}}"{{ with $params.preload }} fetchpriority="high"{{ end }}>


why is this removed?

I thought we needed it because with different ratios the sizes come from the <source> elements. Maybe it is enough there. But I just noticed another problem, there is a layout shift when the sources have a media attribute. I am looking for a solution.

bezoerb · 2022-09-21T22:15:31Z

hugo-modules/core/utils/asset/video.html

 {{- $controls := $params.controls | default false -}}
-{{- $poster := $params.poster | default false -}}
+{{/* Use transparent base64, the real poster should be loaded as <picture> element */}}
+{{- $poster := "data:image/gif;base64,R0lGODlhAQABAIAAAAAAAP///yH5BAEAAAAALAAAAAABAAEAAAIBRAA7" -}}


why not skip the poster attribute completely?

Good question. I asked myself the same question (see TBD). The poster is not a mandatory attribute, but there was some reason... maybe the background is black when the video is not yet loaded? I don't know. I will try to find out and take it out for now.

bezoerb · 2022-09-21T22:18:54Z

templates/theme-default/assets/js/components/video.js

+  observer.observe(video);
 };

 export const initVideo = (root = document) => {


We should call init automatically without the need to be imported & called from main.js

dlemm

Good job @peterschewe
I've found only small things, and more like suggestions.

dlemm · 2022-09-22T05:50:27Z

packages/contentful-migrations/ui/1000000000002-create-c-responsive-media-migration.js

    .validations([
      {
-        in: ['hd', 'rectangle', 'square', 'portrait'],
+        in: ['1x1', '16x9', '9x16', '4x3', '3x4'],


Definitely the better naming and more obviously 👍

I think so too. 👍 Unfortunately, there is still a bug in Sonar that I don't exactly understand yet. I'll have a look. https://sonarcloud.io/project/issues?resolved=false&types=BUG&pullRequest=45&id=jungvonmatt_create-contentful-hugo-app&open=AYNg2o7ptY5uRUoaXYTE

Sounds like sonar thinks that this is a mathematical expression and you just could write 1, I guess..

Thanks @dlemm, that solved it.

dlemm · 2022-09-22T05:59:04Z

packages/recipes/c-video-player/partials/c-video-player.html

+{{- $loop := $params.loop -}}
+{{- $poster_asset := $params.poster -}}
+{{- $controls := $params.controls -}}
+{{- $muted := $params.muted -}}


Do we might need setting a default here? For example true for muted and autoplay or at least for muted since in Safari the autoplay set in Contentful would not work if the editor forgets the muted? Or would this to much control for the markup?

You are right. I think there are more things we would have to rebuild. It's just a 1 to 1 export from the old c-video (just renamed). However, it also has slightly different requirements now. I might have to create a separate issue ticket for that first.

dlemm · 2022-09-22T06:02:15Z

templates/theme-default/assets/scss/components/_c-responsive-media.scss

+    img,
+    svg {
+      vertical-align: top;
+      // max-width: 100%;


Can this comment be deleted?

templates/theme-default/layouts/partials/components/c-responsive-media.html

dlemm · 2022-09-22T07:02:09Z

For the TBD

Caption: Where do we like to use a caption? c-responsive-media vs. m-media

In my opinion it should be placed inside the c-responsive-media since the caption barely changes. So if you use the component somewhere else, would there be another caption?

And another suggestion is that we might need a desktop and a mobile caption since it is possible to maintain two different things now.

peterschewe · 2022-09-22T08:45:12Z

For the TBD

Caption: Where do we like to use a caption? c-responsive-media vs. m-media

In my opinion it should be placed inside the c-responsive-media since the caption barely changes. So if you use the component somewhere else, would there be another caption?

Yes, that is a good point. I can only see a disadvantage if the caption is not directly aligned with the media. For example, the media could be fullscreen and the caption could remain in the content grid. This is also possible in the component. But maybe it's more difficult if it's already in a grid (of the module). The case can probably be ignored...

And another suggestion is that we might need a desktop and a mobile caption since it is possible to maintain two different things now.

I would consider the responsive media as a unit. It is meant to be displayed differently, but the purpose of the content is the same. We also use only one alt text for both media.

sonarqubecloud · 2022-09-22T14:24:10Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

No Coverage information
10.8% Duplication

sonarqubecloud · 2022-12-19T10:30:46Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

No Coverage information
10.8% Duplication

peterschewe added 12 commits September 21, 2022 17:52

feat: Add recipe for a video player component

acc7d84

feat: Add recipe for an image component

1a09d50

refactor: Remove video component

aea566d

refactor: Remove image component

5e0244a

refactor(Storybook): Add local img/video demo assets

44921f7

feat: Add responsive media component

faa5607

refactor: Reorder and clean up migrations

9aea425

refactor: Change config of cssg-plugin-assets

55081a1

docs: Minor corrections in the util comments

d7f8994

refactor(core-utils): Set width and height at <source> instead of at …

7cd4aae

…<img> This change is important if different ratios are defined via sources. The img itself must not have a default size.

refactor(core-utils): Load the video lazy as default and remove image…

7c45fc6

… preloading

fix: Set the breakpoint class correctly

eb006eb

peterschewe changed the title ~~Feature/refactor media components~~ Feature: Responsive media component Sep 21, 2022

bezoerb reviewed Sep 21, 2022

View reviewed changes

dlemm reviewed Sep 22, 2022

View reviewed changes

peterschewe added 4 commits September 22, 2022 10:56

fix: Resolve Sonar warning and use just 1 instead of identical 1/1

5d584ce

chore: Remove TODO comment

d9de07a

refactor: Add caption text

4a33f22

refactor: Localize caption text

95c725c

Delete .DS_Store

9eedefb

Comments

Conversation

peterschewe commented Sep 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Todos

TBD

New component: Responsive media

Uh oh!

bezoerb 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!

dlemm 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!

Uh oh!

dlemm commented Sep 22, 2022

Uh oh!

peterschewe commented Sep 22, 2022

Uh oh!

sonarqubecloud bot commented Sep 22, 2022

Uh oh!

sonarqubecloud bot commented Dec 19, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

peterschewe commented Sep 21, 2022 •

edited

Loading