feat: Add breakpoints option to support toggling classes based on player width. #5471

misteroneill · 2018-10-02T19:46:01Z

Description

This adds a breakpoints option. By default, this option is false meaning this is an opt-in feature.

When passing true, it will use a default set of breakpoints. Or custom breakpoints can be passed if users do not like our breakpoints (or previously-existing style decisions).

Specific Changes proposed

Add breakpoints option.
Adds some new (currently unused) classes: vjs-layout-medium, vjs-layout-large, vjs-layout-x-large, and vjs-layout-huge.
Add updateCurrentBreakpoint and currentBreakpoint methods to the player.
Update css/components/_adaptive.scss
In cleaning up sandbox a bit, I noticed several appearances of vjs-default-skin which was no longer used anywhere. Removed it everywhere.
Add sandbox/responsive.html.example

Requirements Checklist

Feature implemented / Bug fixed
If necessary, more likely in a feature request than a bug fix
- Change has been verified in an actual browser (Chome, Firefox, IE)
- Unit Tests updated or fixed
- Docs/guides updated
Reviewed by Two Core Contributors

gkatsev · 2018-10-02T19:52:44Z

https://deploy-preview-5471--videojs-docs.netlify.com/test-example/responsive.html

misteroneill · 2018-10-02T19:53:05Z

Updating that now. I had local changes that were not reflected there yet.

gkatsev · 2018-10-03T20:05:25Z

Looks like stuff like caption buttons get removed in vjs-layout-small, I think it may be better to hide those buttons in x-small and less but still hide the time displays.

gkatsev

Would be good to add unit tests as well. Otherwise, this is great!

gkatsev · 2018-10-03T19:56:25Z

docs/guides/options.md

+`vjs-layout-x-large` | 1441-2560
+`vjs-layout-huge`    | 2561+
+
+These can be overridden by passing an **ordered** array that looks like this:


would it be better to just have an object?

{ tiny: 210, xmall: 320, small: 425, medium: 768. large: 1140, xlarge: 2560 }

That is, instead of having custom class names as an option, only have the main ones we support

Gotcha, that could make sense. We define the order by class name and then it can be configured via an object where ordering does not matter. I like it. 👍

gkatsev · 2018-10-03T19:57:58Z

sandbox/index.html.example

  <link href="../dist/video-js.css" rel="stylesheet" type="text/css">
  <script src="../dist/video.js"></script>
-  <script src="../node_modules/videojs-flash/dist/videojs-flash.js"></script>


We should leave flash here as it makes it easier to test with since it's already set up. At least for a bit longer still.

Well, my thought was that we have a flash.html.example so it would be confusing as to why it was identical to index.html.example.

sandbox/index.html.example

gkatsev · 2018-10-03T20:01:09Z

src/js/player.js

+   *
+   * @private
+   */
+  updateCurrentBreakpoint() {


if this should be a private method, it should get a postfixed _

gkatsev · 2018-10-03T20:06:56Z

Also, the fullscreen button in x-tiny moves right next to the play toggle, I'd expect it to stay pinned to the right

misteroneill · 2018-10-04T13:34:27Z

Pffft, tests. 😛

(Yeah, I'll do that, got sidetracked)

gkatsev · 2018-10-09T17:47:48Z

Some things I noticed:

The fullscreen button should stay pinned to the right in tiny
looks like when going from tiny to small, the progress control doesn't show up immediately, at least according to the example page. At the top, it has already changed to x-small, but the control bar still looks like tiny. This may be an issue with small->medium as well.
x-small and small seem identical at the moment. Probably worth keeping caption buttons in small.

misteroneill · 2018-10-09T18:56:23Z

looks like when going from tiny to small, the progress control doesn't show up immediately

This is a consequence of the class name highlighting being based on CSS media queries and the playerresize event being throttled.

gkatsev · 2018-10-10T14:53:50Z

Looks like the issue with the second one was that of expectations and there's been an update to better explain it on the page.

…yer width.

misteroneill · 2018-10-10T15:13:11Z

src/css/components/_control-bar.scss

@@ -12,7 +12,7 @@

 // Video has started playing
 .vjs-has-started .vjs-control-bar {
-  @include display-flex;
+  @include display-flex(flex-start, space-between);


This change to the justification should be fine. For all sizes except tiny, the progress bar fills all available space, making the justification pretty much irrelevant. This seems like the right value for other use-cases (without a progress bar).

I wonder if the issue is that the spacer has different settings applied to it in tiny mode:

.vjs-custom-control-spacer { @include flex(auto); } &.vjs-no-flex .vjs-custom-control-spacer { width: auto; }

I'd be more comfortable fiddling with those properties over this one, at first at least.

gkatsev

Awesome, glad we finally have this!

Follow-up for #5471 This makes the breakpoints option and `breakpoints()` method clearer and introduces the responsive option and `responsive()` method, which will turn on the breakpoints. The return value of `currentBreakpoint()` was simplified to only ever return a string (empty if none). Also, added convenience methods: `responsive()`, `getBreakpointClass()`, and `currentBreakpointClass()`.

…yer width. (#5471) This adds a breakpoints option. By default, this option is false meaning this is an opt-in feature. When passing true, it will use a default set of breakpoints. Or custom breakpoints can be passed if users do not like our breakpoints (or previously-existing style decisions). - Add breakpoints option. - Adds some new (currently unused) classes: vjs-layout-medium, vjs-layout-large, vjs-layout-x-large, and vjs-layout-huge. - Add updateCurrentBreakpoint and currentBreakpoint methods to the player. - Update css/components/_adaptive.scss - Add sandbox/responsive.html.example Closes #4371

Follow-up for #5471 This makes the breakpoints option and `breakpoints()` method clearer and introduces the responsive option and `responsive()` method, which will turn on the breakpoints. The return value of `currentBreakpoint()` was simplified to only ever return a string (empty if none). Also, added convenience methods: `responsive()`, `getBreakpointClass()`, and `currentBreakpointClass()`.

axten · 2018-11-26T14:31:51Z

Hey, great feature!
Is it possible to make a new 6.x release? I need this :)
thanks

gkatsev · 2018-11-26T17:01:07Z

@axten yes, I've just been lazy and not releasing 6.x. I'll make sure to get to it this week.

axten · 2018-11-26T18:39:01Z

@gkatsev cool, many thanks!

axten · 2018-12-05T10:59:00Z

It would be cool to have it this week :)

gkatsev · 2018-12-05T18:30:54Z

Oops, totally forgot. I'll be working on this right now.

gkatsev · 2018-12-05T19:34:00Z

@axten 6.13.0 is now out as pre-release with this feature. Please check it out.

gkatsev reviewed Oct 3, 2018

View reviewed changes

brandonocasey approved these changes Oct 9, 2018

View reviewed changes

misteroneill added 6 commits October 10, 2018 11:11

feat: Add breakpoints option to support toggling classes based on pla…

f3d7558

…yer width.

Update responsive.html.example with latest

4050444

Updates based on CR

fa01715

Tests

2af5767

CR

0f9fa4f

CR

57f53c1

misteroneill force-pushed the feat/breakpoints branch from 887f6cc to 57f53c1 Compare October 10, 2018 15:11

misteroneill commented Oct 10, 2018

View reviewed changes

CR

53d0785

gkatsev approved these changes Oct 10, 2018

View reviewed changes

gkatsev merged commit 51bd49f into master Oct 10, 2018

gkatsev deleted the feat/breakpoints branch October 10, 2018 20:53

misteroneill mentioned this pull request Oct 11, 2018

feat: Add responsive option, which enables breakpoints support. #5496

Merged

6 tasks

PoojaPanchal12 mentioned this pull request Aug 31, 2024

[Snyk] Upgrade video.js from 6.12.0 to 6.13.0 CleverTap/vue-video-player#3

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add breakpoints option to support toggling classes based on player width. #5471

feat: Add breakpoints option to support toggling classes based on player width. #5471

misteroneill commented Oct 2, 2018 •

edited

Loading

gkatsev commented Oct 2, 2018

misteroneill commented Oct 2, 2018

gkatsev commented Oct 3, 2018

gkatsev left a comment

gkatsev Oct 3, 2018

gkatsev Oct 3, 2018

misteroneill Oct 4, 2018

gkatsev Oct 3, 2018

misteroneill Oct 4, 2018

gkatsev Oct 3, 2018

gkatsev commented Oct 3, 2018

misteroneill commented Oct 4, 2018

gkatsev commented Oct 9, 2018 •

edited by misteroneill

Loading

misteroneill commented Oct 9, 2018 •

edited

Loading

gkatsev commented Oct 10, 2018

misteroneill Oct 10, 2018

gkatsev Oct 10, 2018

misteroneill Oct 10, 2018

gkatsev left a comment

axten commented Nov 26, 2018 •

edited

Loading

gkatsev commented Nov 26, 2018

axten commented Nov 26, 2018

axten commented Dec 5, 2018 •

edited

Loading

gkatsev commented Dec 5, 2018

gkatsev commented Dec 5, 2018

feat: Add breakpoints option to support toggling classes based on player width. #5471

feat: Add breakpoints option to support toggling classes based on player width. #5471

Conversation

misteroneill commented Oct 2, 2018 • edited Loading

Description

Specific Changes proposed

Requirements Checklist

gkatsev commented Oct 2, 2018

misteroneill commented Oct 2, 2018

gkatsev commented Oct 3, 2018

gkatsev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkatsev commented Oct 3, 2018

misteroneill commented Oct 4, 2018

gkatsev commented Oct 9, 2018 • edited by misteroneill Loading

misteroneill commented Oct 9, 2018 • edited Loading

gkatsev commented Oct 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkatsev left a comment

Choose a reason for hiding this comment

axten commented Nov 26, 2018 • edited Loading

gkatsev commented Nov 26, 2018

axten commented Nov 26, 2018

axten commented Dec 5, 2018 • edited Loading

gkatsev commented Dec 5, 2018

gkatsev commented Dec 5, 2018

misteroneill commented Oct 2, 2018 •

edited

Loading

gkatsev commented Oct 9, 2018 •

edited by misteroneill

Loading

misteroneill commented Oct 9, 2018 •

edited

Loading

axten commented Nov 26, 2018 •

edited

Loading

axten commented Dec 5, 2018 •

edited

Loading