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

Already on GitHub? Sign in to your account

Fluid 4839 #97

Merged
merged 4 commits into from Dec 6, 2012

Conversation

Projects
None yet
2 participants
Member

michelled commented Nov 28, 2012

@colinbdclark, this pull request puts back basic support for Youtube videos. I've decided to go with letting mediaelement.js take care of this for us instead of using our own, home grown solution. There are still 4 outstanding issues that need to be addressed with the integration, but this pull request at least gets rid of the errors we are currently seeing in master.

I'd like your opinion on the default video size. Right now, it's specified in the VideoPlayer.css file and needs to be overridden by an integrator. You can see my overrides in Mammals.css. Do you think this is a reasonable API?

The issues to be addressed are:

FLUID-4853: Populate the start and end times for Youtube videos
FLUID-4854: Make captions show for Youtube videos
FLUID-4855: Remove Youtube watermark and tooltip from video player
FLUID-4856: Fix 'jank' when using a Youtube video

@jobara jobara and 1 other commented on an outdated diff Dec 4, 2012

demos/Mammals.js
- {
- src: "videos/PolarMammalAdaptations/PolarMammalAdaptations.mp4",
- type: "video/mp4"
- },
- {
- src: "videos/PolarMammalAdaptations/PolarMammalAdaptations.webm",
- type: "video/webm"
- },
+ // {
+ // src: "videos/PolarMammalAdaptations/PolarMammalAdaptations.mp4",
+ // type: "video/mp4"
+ // },
+ // {
+ // src: "videos/PolarMammalAdaptations/PolarMammalAdaptations.webm",
+ // type: "video/webm"
+ // },
@jobara

jobara Dec 4, 2012

Owner

Maybe instead of commenting these video sources out, we could move up the youtube one to the top of the array.

@michelled

michelled Dec 4, 2012

Member

It turns out that changing the order does not force the youtube video to be used. I think I should probably revert this change though. We can always comment it out locally to test the youtube support.

@jobara jobara and 1 other commented on an outdated diff Dec 4, 2012

js/VideoPlayer.js
@@ -548,7 +540,10 @@ https://github.com/fluid-project/infusion/raw/master/Infusion-LICENSE.txt
} else if (key === "videoPlayer") {
that.container.append(res[key].resourceText);
that.refreshView();
- that.locate("video").attr("aria-label", that.options.strings.videoTitlePreface + ": " + that.options.videoTitle);
+ var video = that.locate("video");
+ video.attr("aria-label", that.options.strings.videoTitlePreface + ": " + that.options.videoTitle);
+ video.attr("width", video.css("width"));
+ video.attr("height", video.css("height"));
@jobara

jobara Dec 4, 2012

Owner

Why is it that we need to set the width and height as an attribute from the computed css values?

@michelled

michelled Dec 4, 2012

Member

One of the browsers - I think it was Safari - was prioritizing the width and height attributes over the CSS width and height properties. As a result, the video was displayed smaller than the container and it all looked broken.

@jobara jobara and 1 other commented on an outdated diff Dec 4, 2012

js/VideoPlayer.js
@@ -548,7 +540,10 @@ https://github.com/fluid-project/infusion/raw/master/Infusion-LICENSE.txt
} else if (key === "videoPlayer") {
that.container.append(res[key].resourceText);
that.refreshView();
- that.locate("video").attr("aria-label", that.options.strings.videoTitlePreface + ": " + that.options.videoTitle);
+ var video = that.locate("video");
+ video.attr("aria-label", that.options.strings.videoTitlePreface + ": " + that.options.videoTitle);
@jobara

jobara Dec 4, 2012

Owner

Perhaps we could use a string template for this instead.

@michelled

michelled Dec 4, 2012

Member

After looking at this, I'm wondering why the 'videoTitlePreface' doesn't include the ':'. If it did, we'd just be concatenating 2 strings which I think would be fine. What do you think?

@jobara

jobara Dec 5, 2012

Owner

I guess we'd also need to add in the space still, or also make that a requirement for the preface so that we don't end up with "video:title".

Owner

jobara commented Dec 4, 2012

I found that the youtube videos don't work in full screen when run in webkit browsers. I've filed a jira for this.
http://issues.fluidproject.org/browse/FLUID-4858

Member

michelled commented Dec 4, 2012

Hi Justin,

I had replied inline to your comment above regarding flipping the order. It turns out that changing the order does not force the youtube video to be used. I actually think this is a good thing - that the code determines the best option to use based on your browser's support.

Owner

jobara commented Dec 5, 2012

@michelled that does make sense. However, I suppose it still leaves a question about what to do for this case, since I take it you want to force this video to be demoed with the one from youtube.

@jobara jobara merged commit b3307c9 into fluid-project:master Dec 6, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment