Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Video: play Youtube videos #4963

Open
wants to merge 6 commits into
base: rel-1.3
Choose a base branch
from
Open

Video: play Youtube videos #4963

wants to merge 6 commits into from

Conversation

steal4life
Copy link
Collaborator

@steal4life steal4life commented Aug 31, 2023

Closes #4872

@steal4life
Copy link
Collaborator Author

So the idea is that the user just enters the id from youtube, the plan is to add an explanation to users where they can find that id and use it to add the desired video

@steal4life steal4life self-assigned this Aug 31, 2023
@steal4life
Copy link
Collaborator Author

so a youtube video can't be rendered using plyr without the /embed/ part of the link and what comes after the YoutubeVideoID, based on other cases I've found while working on this, I think it's the most optimal, because in this way user can play any video they want from YouTube

@stsrki stsrki changed the base branch from master to rel-1.3 August 31, 2023 12:17
@steal4life
Copy link
Collaborator Author

therefore, the user can reduce the width and height of the container in which YouTube is located, as for other parameters that are characteristic of the video, YouTube already covers it, I don't know if we should adjust that parameters when user is using youtube video and how can we do that?

@steal4life steal4life changed the title Wip enable youtube in <Video> closes enable youtube in <Video> Sep 4, 2023
@stsrki stsrki changed the title closes enable youtube in <Video> Video: play Youtube videos Sep 15, 2023
///<summary>
/// ID of youtube video link.
/// </summary>
public string YoutubeVideoID;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this have the setter as public? Seems like we set this ourselves and should not allow anyone to override it?

Copy link
Contributor

Choose a reason for hiding this comment

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

And should we really expose something specific to Youtube like this? Which we don't seem to require to do anything with it, and are adding specific regex logic that could perhaps change with time? i.e youtube changes the way they do ids or something?

@@ -41,6 +51,7 @@ public override async Task SetParametersAsync( ParameterView parameters )
ProtectionHttpRequestHeaders = new { Changed = protectionHttpRequestHeadersChanged, Value = paramProtectionHttpRequestHeaders },
CurrentTime = new { Changed = currentTimeChanged, Value = paramCurrentTime },
Volume = new { Changed = volumeChanged, Value = paramVolume },
YoutubeVideoSource = new { Changed = youtubeVideoSourceChanged, Value = paramYoutubeVideoSource }
Copy link
Contributor

Choose a reason for hiding this comment

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

@stsrki @steal4life
So this YoutubeVideoSource is an "option" of the underlying plyr library?

What if we instead added a SourceType enum instead?

SourceType.Default
SourceType.Youtube

and the user still defines the source in the same Source parameter, and we act accordingly to the defined SourceType internally?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants