Skip to content
This repository has been archived by the owner on May 8, 2024. It is now read-only.

feat: add refactor + tests #19

Merged
merged 1 commit into from
Jan 12, 2023
Merged

feat: add refactor + tests #19

merged 1 commit into from
Jan 12, 2023

Conversation

luwes
Copy link
Contributor

@luwes luwes commented Jan 10, 2023

@luwes luwes self-assigned this Jan 10, 2023
Copy link

@gkatsev gkatsev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of comments, no blockers. We should set up deploy previews on these repos.

Comment on lines +22 to +23
top: 0;
left: 0;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively, use inset which allows setting all 4 props at once

Suggested change
top: 0;
left: 0;
inset: 0;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll keep it in mind.
the iframe still has a 100% width and height which in some cases was preferred over right: 0 and bottom: 0. I forget the exact details around it.

else this.removeAttribute('playsinline');
}

get poster() {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to support poster? I'd expect that either the youtube embed will be showing it or that users will use media-chrome's poster image element.

Comment on lines +392 to +402
set volume(val) {
if (this.volume == val) return;
this.loadComplete.then(() => {
this.api?.setVolume(val * 100);
});
}

get volume() {
if (!this.isLoaded) return 1;
return this.api?.getVolume() / 100;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we need to keep a cache of values like for volume so that you can get the updated value immediately, since the operation is async. Probably not a blocker, but something to think about.

@@ -0,0 +1,542 @@
// https://developers.google.com/youtube/iframe_api_reference

const EMBED_BASE = 'https://www.youtube.com/embed';
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be worth supporting youtube-nocookie.com when enabled via some flag so that it doesn't use cookies
https://support.google.com/youtube/answer/171780?hl=en-GB#zippy=%2Cturn-on-privacy-enhanced-mode

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not a blocker and could be implemented later on.

@luwes luwes merged commit 61fd549 into muxinc:master Jan 12, 2023
@luwes luwes deleted the refactor2 branch January 12, 2023 15:32
@luwes luwes mentioned this pull request Jan 12, 2023
luwes added a commit to luwes/media-chrome that referenced this pull request Jan 12, 2023
luwes added a commit to muxinc/media-chrome that referenced this pull request Jan 12, 2023
luwes added a commit to luwes/playerx that referenced this pull request Jan 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants