feat: new slide-video component

[nweldev]

I love gifs! But I don't like being in front of a distracted audience... So, during my talks, I only show a "gif" (mp4) once. Well, sometimes more than once, but at least, I need to control play/pause (I could have done it better here, but ... well, it's late, and it's good enough for what I need right now).

BTW, I created a video component ...

... instead of refactoring slide-gif because (I think...) it doesn't handle mp4.

IMO, it could be a good idea to add a "lazy load video source" component and then merge slide-gif and slide-video. Let me know what you think.

alternative approache

I first tried to control play/pause on swipe doing the following :

<deckgo-slide-content class="dark video-slide">
   <video muted playsinline slot="background" class="full">
     <source src="assets/gif/fire-explosion.mp4" type="video/mp4">
    </video>
</deckgo-slide-content>

export const videoPlayOnSwipe = () => {
  const slides = document.getElementsByClassName('video-slide');
  [...slides].forEach(async (/** @type {HTMLDeckgoSlideContentElement} */ slide) => {
    await slide.afterSwipe();
    slide.querySelector('video').play();
  });
}

But afterSwipe and beforeSwipe always returns the same Promise. I don't think they're supposed to be used this way anyway 😅. Using the deck for that instead also feels like a workaround.

IIWM, I would add a swipe event to DeckdeckgoSlide in order to solve this kind of issues (when people needs to use next/prev inside a slide but just for a one-shot, making the creation of a new component useless). It would be nice for animations for example.

[peterpeterparker]

Awesome, thx for this other PR 👍

Spontaneously I would say...

Having a separate template for video aka not refactoring the gif one could make sense, maybe some person would like someday to drop their own video assest in a slide? Like having locally in your assets folder. I like it.

About the play/pause approach, I think we should follow the same approach as the youtube slide template (exposing two methods play/pause). This is an advantage as I'll then be able to link with our remote control 🚀

Maybe we could add an interface DeckdeckgoVideoSlide which declare these methods.

Finally about the code, I would not declare a slot but rather add the video tag in the content.

I could merge this PR not directly in master but in a new branch, I do some changes and I ask you for a review, ok?

[peterpeterparker]

@noelmace I've developed several changes in branch slide-video (https://github.com/deckgo/deckdeckgo/tree/slide-video), could you have a look?

would that work for you?

if yes, I'll then add a "play" and "pause" button to the starter kit and also connect the slide with the remote control

Will also add a new interface like the following:

import {DeckdeckgoSlide} from './deckdeckgo-slide';

export interface DeckdeckgoSlidePlayable extends DeckdeckgoSlide {
    play(): Promise<void>;

    stop(): Promise<void>;

    toggle(): Promise<void>;
}

And of course add the doc. Let me know if that works and I'll continue. If not, let me know what you rather like to have differently?

Thx in advance for your feedback

[nweldev]

IIWM, I would prefer to avoid duplicating elements behaviors for better readability and maintainability. Here, it doesn't allow to call a bunch of methods and attributes of HTMLMediaElement (e.g. currentTime) without querying the shadow root of the component. It also doesn't permit to add alternative video sources. Could exposing a "video" getter in order to play/pause the video given using a slot=video could be ok for you? This is more an opinionated approach of Web Components architecture than unbiased feedback anyway. Let me know what you think, I love this kind of debates 😉

BTW, why are you using:

beforeSwipe(_enter: boolean, _reveal: boolean): Promise<boolean> {
  return new Promise<boolean>((resolve) => {
    resolve(true);
  });
}

instead of

async beforeSwipe(_enter: boolean, _reveal: boolean): Promise<boolean> {
  return true;
}

IMO, both are the same, but using async is easier to read.

About the behavior itself, I don't have any strong opinion about it for now. I'll give it a try with the remote controller once it's implemented. I guess it will require to look at the phone in order to find the button and then tap, or to add a pretty big button which would make the speaker notes really small, but it makes it more consistent with other slides so it may be a better choice indeed.

[peterpeterparker]

Thx for the feedback 👍

You mark some valid points, specially on the duplicate behavior. It's maybe a question of approach. I like that "obfuscate" way as it makes the component easier to approach, specially if you don't know well HTML, and furthermore, it's kind of an advantage for our editor. We save html in our database, having the video "inside" the component and not provided as slot equals less code and less data to save, of course in case we would like one day to add the component to the editor.

But you are right, doing so, we "miss" a lot of features of the HTMLMediaElement and it would be maybe suboptimal to duplicate all attributes.

So yes I think your idea of exposing a "video" getter would be a good solution. The kind of solution we like in Switzerland, in the center, no one is really super sad, no one is really super happy 🤣

async getVideo() {
      return this.el.shadowRoot.querySelector('video)';
}

Ok like that?

About async beforeSwipe yes you are right, "les habitudes ont la vie dure hein". We could change that but I would prefer in another PR (in order to update the interface and if needed the slides or components which are inheriting it that way).

About remote, right now it is not easy to find the action but it will undergo a "big" UX and design refactoring. One of the idea is to have bigger buttons and notably a bigger play/pause button you can't miss (see #228)

[nweldev]

#335 (comment) LGTM

[nweldev]

Maybe open a new PR for that, as this one now appears merged.

[peterpeterparker]

Cool 👍

Of course I'll open a PR, clean some stuffs in the branch and will do. Obviously will link it here.

Thx for the inputs :)

[peterpeterparker]

Still need to develop the doc but the following PR is #337