search

mitodl / ocw-hugo-themes

A Hugo theme for building OCW websites
BSD 3-Clause "New" or "Revised" License
5 stars 4 forks source link

Video pages layout width-fix #1369

Closed ibrahimjaved12 closed 8 months ago

ibrahimjaved12 commented 8 months ago

What are the relevant tickets?

Closes https://github.com/mitodl/hq/issues/2921

Description (What does it do?)

Removes max-width: 100vh from video pages styling to allow the content to spread over all of the div body. This means, not only the text, but videos will now take up entire div width too.

Screenshots

Refer to https://github.com/mitodl/ocw-hugo-themes/pull/1369#issuecomment-1993747002

How can this be tested?

Checkout to this branch, check any course that has video lectures (the default course also does -- yarn start course). Check embed video pages as well. An example of embed video is in course 18.06sc-fall-2011 in the Instructor Insights page.

For offline course theme, run: yarn build ~/path/ocw-content-rc/18.06sc-fall-2011/ ~/path/ocw-hugo-projects/ocw-course-v2/config-offline.yaml And then open index.html from dist directory of the course, and navigate to videos. Replace course-id with course of your choice.

github-actions[bot] commented 8 months ago

Netlify Deployments:
www: https://ocw-hugo-themes-www-pr-1369--ocw-next.netlify.app/
Course v2: https://ocw-hugo-themes-course-v2-pr-1369--ocw-next.netlify.app/

pdpinch commented 8 months ago

Does the div have a maximum width?

I know that on desktop views, we don't like to extend the text (and the video) too much. At really wide widths, it becomes hard to read (See, for example, the layouts of medium.com or nytimes.com)

ibrahimjaved12 commented 8 months ago

Does the div have a maximum width?

I know that on desktop views, we don't like to extend the text (and the video) too much. At really wide widths, it becomes hard to read (See, for example, the layouts of medium.com or nytimes.com)

@pdpinch The webpage has dynamic max-width (fixed max widths, but different for different screens) For desktop view, we have a max-width of 1447px for the entire webpage (which is centered)

From this, the video area div has max-width of 75% of the total width

image

Even if we close the right course info bar, then the video area takes its space, but only its space, not more than that

So our design is very similar to those websites

image image

Previously we had this exact problem with the header and footers - that they would just spread around on left/right corners, for larger screens. But we fixed that recently. Although I think we may be due a refactor for the overall page height/layout, as illustrated in this ticket I just created: https://github.com/mitodl/hq/issues/3759 -- but that is off-topic here.

Here are full screenshots: image image

image image

image

ChristopherChudzicki commented 8 months ago

@ibrahimjaved12: The max-width: 100vh was set in #898 to fix #865 ... Basically, the video playwer was getting too large and people couldn't see the controls.

From your screenshots, it looks like this change does NOT re-introduce #865 (good). But IMO worth double checking the reproduction steps in #865. Note: When #865 was reported, we were using a different video player, and subsequently switched to the YouTube player. So, assuming #865 has not resurfaced, that's one possible explanation for why.

ibrahimjaved12 commented 8 months ago

Thanks for sharing that @ChristopherChudzicki

I can see why we do not want larger sized videos. The video in the ticket you shared, currently we can see it's moderately sized, and the transcripts are shown 100% on the screen - whether you have Course Info menu shown or not.

https://live-qa.ocw.mit.edu/courses/18-02sc-multivariable-calculus-fall-2010/resources/changing-the-order-of-integration/

But under this PR's fix, viewing the entire transcript area would be a problem. Especially with Course Info menu off - then the video takes the entire viewport.

I'm going to revisit this, and see what we can do about this. The issue was reported for a mobile phone, one way we could go about this would be to limit only the video to a width - without limiting the entire page area.

This would fix the text problem, without changing the video player dimensions. image

@mbilalmughal What do you think we should do.

ibrahimjaved12 commented 8 months ago

Kind of restrained by video player's entire transcript to be seen requirement, this is what I thought of. Video player stays same as before, but we allow the textual content to take screen's allocated max-width (max-width of xs, s, md, lg, etc)

image

Basically you will see in the first pic in this screenshot, previously the text was same width as video player, but now its taking all the space it needs.

mbilalmughal commented 8 months ago

@ibrahimjaved12 This looks appropriate to me, The video little narrower. In order to adjust the controls and content should be stretched to 100%. 👍

ibrahimjaved12 commented 8 months ago

This PR is again ready for review

pdpinch commented 8 months ago

Ok, I think this is the best compromise we can get. It allows for the video and the transcript to both be in view.

If the user really wants a full-wdith video, they can tap the full screen button.

ibrahimjaved12 commented 8 months ago

👍 This looks fine to me. I find myself wondering if we could take a different approach to fullscreening the video though. According to https://developers.google.com/youtube/player_parameters, we can pass fs=0 to disable the fullscreen button within the YouTube player. Then, in theory, we could provide our own fullscreen button. We could then use the :fullscreen pseudo class to define our own fullscreen layout. This layout could place the transcript on the right side of the video in landscape, and at the bottom of the video in portrait. In theory, anyway. I think that this could maybe be worth it, but it depends on how many people watch OCW videos in this manner. As @pdpinch mentioned, they can fullscreen the video and turn on closed captioning in YouTube which will provide the transcript. The only functionality this doesn't offer is scrolling through the transcript and clicking a line to seek there in the video. If we provided a custom fullscreen layout, then this would be possible. For example, the Twitch video player provides similar functionality with their "theater mode" button which makes the video take up all available space in the browser window aside from a column on the right hand side for the chat. We could create a similar layout, but with a transcript instead of live stream chat.

Thanks @gumaerc. I like the idea. For now, I'm going to merge this PR. But I'll R&D / experiment on this and see how we could go with this.