AlmaLinux / almalinux.org

almalinux.org official web site sources.
https://almalinux.org
Creative Commons Attribution Share Alike 4.0 International
39 stars 49 forks source link

horizontal overflow issue on mobile caused by YouTube embedded videos, #564

Closed Noam-Alum closed 1 month ago

Noam-Alum commented 1 month ago

The website on mobile has a horizontal overflow issue caused by YouTube embedded videos:

image

I found the issue in the following pages:

https://almalinux.org/blog/2023-12-14-2023-highlights/ https://almalinux.org/blog/2024-01-16-video-contributions/ https://almalinux.org/blog/2024-05-06-announcing-94-stable/ https://almalinux.org/blog/2024-05-01-almalinux-day-germany-recap/

Important to note that these pages have a youtube video embedded to them and are not part of this issue:

https://almalinux.org/blog/2024-02-29-feb-event-recap/ https://almalinux.org/blog/2024-02-01-aldt-recap/

This is an iframe with the issue:

<iframe width="560" height="315" src="https://www.youtube.com/embed/DNMnajmyLaA?si=8zE5H90evplzV8-t" title="AlmaLinux Binary Compatibility Q & A" frameborder="0" allow="accelerometer; autoplay; clipboard-write; encrypted-media; gyroscope; picture-in-picture; web-share" allowfullscreen></iframe>

And this is without:

<iframe width="560" height="315" src="https://www.youtube.com/embed/HgZKLs5ItH4?si=cvzYgbjZurFP0EIR" title="Elkhan Mammadli: AlmaLinux: How we automated testing without inventing the wheel and instead improving it" frameborder="0" allow="accelerometer; autoplay; clipboard-write; encrypted-media; gyroscope; picture-in-picture; web-share" allowfullscreen style="max-width: 100%;"></iframe>

Seems like the only difference is that the one without the issue uses style="max-width: 100%;".


Maybe to embed the videos properly we can use a shortcode to render the videos: https://gohugo.io/content-management/shortcodes/#youtube

Let me know if we can use this kind of solution and I'll create a PR for this issue.

codyro commented 1 month ago

I brought this up a few months ago, and @bennyvasquez mentioned an issue that needed to be addressed before we could use them.

https://github.com/AlmaLinux/almalinux.org/pull/444#discussion_r1453694873 (expand the resolved conversation)

I found that, but the shortcode doesn't allow me to adjust the size of it inline like this, and will just expend to be the full size of the blog post width. It would be cool to expand the shortcode implementation with additional CSS at some point, but that wasn't a today problem. :D

If we can get this fixed, it’d significantly improve the UX.

@mattlasheboro is this something that's a pain to fix?

Noam-Alum commented 1 month ago

I brought this up a few months ago, and @bennyvasquez mentioned an issue that needed to be addressed before we could use them.

#444 (comment) (expand the resolved conversation)

I found that, but the shortcode doesn't allow me to adjust the size of it inline like this, and will just expend to be the full size of the blog post width. It would be cool to expand the shortcode implementation with additional CSS at some point, but that wasn't a today problem. :D

If we can get this fixed, it’d significantly improve the UX.

@mattlasheboro is this something that's a pain to fix?

We can take the shortcode default code and change it to include hight and width:

/layouts/shortcodes/youtube.html:

{{- /*
    Renders an embedded YouTube video.

    @param {bool} [allowFullScreen=true] Whether the iframe element can activate full screen mode.
    @param {bool} [autoplay=false] Whether to automatically play the video. Forces mute to be true.
    @param {string} [class] The class attribute of the wrapping div element. When specified, removes the style attributes from the iframe element and its wrapping div element.
    @param {bool} [controls=true] Whether to display the video controls.
    @param {int} [end] The time, measured in seconds from the start of the video, when the player should stop playing the video.
    @param {string} [id] The video id. Optional if the id is provided as first positional argument.
    @param {string} [loading=eager] The loading attribute of the iframe element.
    @param {bool} [loop=false] Whether to indefinitely repeat the video. Ignores the start and end arguments after the first play.
    @param {bool} [mute=false] Whether to mute the video. Always true when autoplay is true.
    @param {int} [start] The time, measured in seconds from the start of the video, when the player should start playing the video.
    @param {string} [title] The title attribute of the iframe element. Defaults to "YouTube video".
    @param {string} [width="100%"] The width of the video.
    @param {string} [height="56.25%"] The height of the video.

    @returns {template.HTML}

    @reference https://developers.google.com/youtube/player_parameters

    @example {{< youtube 0RKpf3rK57I >}}
    @example {{< youtube id=0RKpf3rK57I loading=lazy start=30 >}}
    */}}

    {{- $pc := .Page.Site.Config.Privacy.YouTube }}
    {{- $remoteErrID := "err-youtube-remote" }}
    {{- if not $pc.Disable }}
      {{- with $id := or (.Get "id") (.Get 0) }}

        {{/* Set defaults. */}}
        {{- $allowFullScreen := "allowfullscreen" }}
        {{- $autoplay := 0 }}
        {{- $class := "" }}
        {{- $controls := 1 }}
        {{- $end := 0 }}
        {{- $loading := "eager" }}
        {{- $loop := 0 }}
        {{- $mute := 0 }}
        {{- $start := 0 }}
        {{- $title := "YouTube video" }}
        {{- $width := "100%" }}
        {{- $height := "56.25%" }}

        {{- /* Get arguments. */}}
        {{- if in (slice "false" false 0) ($.Get "allowFullScreen") }}
          {{- $allowFullScreen = "" }}
        {{- else if in (slice "true" true 1) ($.Get "allowFullScreen") }}
          {{- $allowFullScreen = "allowfullscreen" }}
        {{- end }}
        {{- if in (slice "false" false 0) ($.Get "autoplay") }}
          {{- $autoplay = 0 }}
        {{- else if in (slice "true" true 1) ($.Get "autoplay") }}
          {{- $autoplay = 1 }}
        {{- end }}
        {{- if in (slice "false" false 0) ($.Get "controls") }}
          {{- $controls = 0 }}
        {{- else if in (slice "true" true 1) ($.Get "controls") }}
          {{- $controls = 1 }}
        {{- end }}
        {{- if in (slice "false" false 0) ($.Get "loop") }}
          {{- $loop = 0 }}
        {{- else if in (slice "true" true 1) ($.Get "loop") }}
          {{- $loop = 1 }}
        {{- end }}
        {{- if in (slice "false" false 0) ($.Get "mute") }}
          {{- $mute = 0 }}
        {{- else if or (in (slice "true" true 1) ($.Get "mute")) $autoplay }}
          {{- $mute = 1 }}
        {{- end }}
        {{- $class := or ($.Get "class") $class }}
        {{- $end := or ($.Get "end") $end }}
        {{- $loading := or ($.Get "loading") $loading }}
        {{- $start := or ($.Get "start") $start }}
        {{- $title := or ($.Get "title") $title }}
        {{- $width := or ($.Get "width") $width }}
        {{- $height := or ($.Get "height") $height }}

        {{- /* Determine host. */}}
        {{- $host := cond $pc.PrivacyEnhanced "www.youtube-nocookie.com" "www.youtube.com" }}

        {{- /* Set styles. */}}
        {{- $divStyle := printf "position: relative; width: %s; padding-bottom: %s; height: 0; overflow: hidden;" $width $height }}
        {{- $iframeStyle := "position: absolute; top: 0; left: 0; width: 100%; height: 100%; border:0;" }}
        {{- if $class }}
          {{- $iframeStyle = "" }}
        {{- end }}

        {{- /* Set class or style of wrapping div element. */}}
        {{- $divClassOrStyle := printf "style=%q" $divStyle }}
        {{- with $class }}
          {{- $divClassOrStyle = printf "class=%q" $class }}
        {{- end }}

        {{- /* Define src attribute. */}}
        {{- $src := printf "https://%s/embed/%s" $host $id }}
        {{- $params := dict
          "autoplay" $autoplay
          "controls" $controls
          "end" $end
          "mute" $mute
          "start" $start
          "loop" $loop
        }}
        {{- if $loop }}
          {{- $params = merge $params (dict "playlist" $id) }}
        {{- end }}
        {{- $s := slice }}
        {{- range $k, $v := $params }}
          {{- $s = $s | append $k }}
          {{- $s = $s | append $v }}
        {{- end }}
        {{- with querify $s }}
          {{- $src = printf "%s?%s" $src . }}
        {{- end }}

        {{- /* Set iframe attributes. */}}
        {{- $iframeAttributes := dict
          "allow" "accelerometer; autoplay; clipboard-write; encrypted-media; gyroscope; picture-in-picture; web-share"
          "allowfullscreen" $allowFullScreen
          "loading" $loading
          "referrerpolicy" "strict-origin-when-cross-origin"
          "src" $src
          "style" $iframeStyle
          "title" $title
        }}

        {{- /* Render. */}}
        <div {{ $divClassOrStyle | safeHTMLAttr }}>
          <iframe
            {{- range $k, $v := $iframeAttributes }}
              {{- if $v }}
                {{- printf " %s=%q" $k $v | safeHTMLAttr }}
              {{- end }}
            {{- end }}
          ></iframe>
        </div>
      {{- else }}
        {{- errorf "The %q shortcode requires an id argument. See %s" .Name .Position }}
      {{- end }}
    {{- end }}

I tested this and it seems to be working nicely, the only issue being that when you are on mobile the video is too small. Screenshot from 2024-05-23 07-17-51

This is how it looks on a computer: Screenshot from 2024-05-23 07-18-32

This is how it looks as of now (With bennys resizing): image

example of what I used:

{{< youtube id="DNMnajmyLaA" width="45%" height="25%" autoplay="true" controls="true" mute="false" title="YouTube video player" >}}
mattlasheboro commented 1 month ago

Noam's solution looks great to me and fixes the overflow issue. Moving to shortcodes for the embed is definitely the right move for the future.

I think a simple solution for mobile is the best bet. We could do something like this:

<div data-role="yt-embed" {{ $divClassOrStyle | safeHTMLAttr }}>

And add the style:

@media screen and (max-width:991px) { [data-role="yt-embed"] { min-width:90%; } }

If we REALLY needed to be able to set a percentage width for mobile that differs from desktop we could add a separate parameter (m-width or something) and then including a style block like:

<style> @media screen and (max-width:991px) { [data-role="yt-embed"] { min-width: {{$.Get "m-width"}}; aspect-ratio: 16 / 9; height: auto; } } </style>

I'd stick with the former unless there is a huge need for granular control over video widths, though.

bennyvasquez commented 1 month ago

As a note, I think the max-widths in the embeds that were causing the problem were accidental, but I love that it's lead to this getting eyes on it! Provided feedback on the PR, but I wanted to make sure I said something here, too!