-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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
New Base Theme #1999
New Base Theme #1999
Conversation
9c5437e
to
24851cc
Compare
@@ -94,7 +94,7 @@ vjs.TimeDivider = vjs.Component.extend({ | |||
|
|||
vjs.TimeDivider.prototype.createEl = function(){ | |||
return vjs.Component.prototype.createEl.call(this, 'div', { | |||
className: 'vjs-time-divider', | |||
className: 'vjs-time-controls vjs-time-divider', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
vis-time-controls
should be singular, yeah?
Another giant 5.0 PR, good luck everybody! :) Good stuff Matt. For everyone, the commits has a decent list of the overall changes. @mmcc, can you talk about some of the motivations behind all of this? I'm thinking specifically about breaking up the files, the variables file, and anything else related. Also since every line of CSS was moved, it's hard to tell what styles changed. What can we do to start a skin migration guide? |
- 0.10 | ||
- 0.10 | ||
before_install: | ||
- gem install sass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is gem install sass
a dependency for everyone or just for Travis?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use libsass
instead? I really don't want to need to have to deal with gems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now this PR is using grunt-contrib-sass, which uses Ruby sass. However, we can try using grunt-sass instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whatever doesn't require me to ever touch gem
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💅
-moz-box-ordinal-group: $value; | ||
-ms-flex-order: $value; | ||
-webkit-order: $value; | ||
order: $value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is order red here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
¯_(ツ)_/¯
…default (and hidden via css
background: rgba($secondary-bg, 0.1); | ||
} | ||
|
||
.video-js .vjs-slider-handle.vjs-seek-handle { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we reorder the includes so this doesn't need the double class? It doesn't look like slider-handle currently sets these values anyway, but it probably should so you can use a default slider.
|
||
// Video has started playing AND user is inactive | ||
.video-js.vjs-has-started.vjs-user-inactive.vjs-playing .vjs-control-bar { | ||
@include display-flex; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think @include display-flex
is actually needed here again.
Also added a grunt task specifically for skin development (only watches sass file changes and runs sass)
@include flex(auto); | ||
@include display-flex(center); | ||
|
||
@include transition(all 0.4s); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the progress bar isn't growing and shrinking anymore then this isn't needed.
This allows the poster image to toggle play / pause on audio-only sources
All the |
Here's the rest of the notes I took on changes, to be added to the skin migration guide.
These probably aren't finished/correct
|
A few notes from Steve's initial comment that I never answered before.
At the most basic level, I've personally wanted to switch to Sass for a while. A lot of the original decision was motivated by being able to use Less in the browser, which is irrelevant these days. This skin is simpler in some ways, such as being able to let the handle overlap some of the progress bar to avoid the old issues with the diamond. The new flexbox / table layout gives us the flexibility we had before in terms of being able to add new items to the control bar, without needing too much hackery (besides the whole table thing). Re: breaking up the files, the old LESS file was massive, which meant doing anything at all in the file was a pain. Particularly with the ES6 / Browserify changes, I think the Sass updates just keep the skin in line with the rest of the project. |
@mmcc I'm interested in the adaptive layout stuff (vjs-layout-tiny and friends) that were added with this. What was the thought behind that ? It's not yet in use right ? |
@hartman So the thought was we'd use JS to determine the width of the player on resize and add these classes programmatically. We decided not to do that (yet? maybe?), but I left them in for folks that wanted to use them on their own. Anyway, the thought behind it was that VJS kinda sucked at smaller sizes. Once it got down to a certain point things started falling off the control bar and generally looked bad. Most of those problems should be fixed with the new layout since it resizes better, but at smaller sizes it still looks waaaay too busy imho. So, ultimately, this was an effort to make smaller players more usable. Thoughts? Would you like to see this added programmatically or is it enough just to have the classes there? |
@mmcc Experimented a bit with doing this programatically. Took a lot more than I anticipated, though the code can be cleaned up a bit more. But, it works: http://jsfiddle.net/TheDJ/5raorqgy/ |
Opening the PR so we can start talking about it, but this needs a little more work before it can be pulled in. We should talk about things like:
I'm almost certain I blew away some necessary stuff in the Gruntfile when I was rebasing from master, but I didn't want to dig through the various tasks and mess with it. There are probably css-specific things from the build task specifically that got blown away. Sorry. 😓
Lastly, this needs to be tested a ton across all the browsers and devices. Seriously. I'm using
flexbox
with atable
fallback, so it's not like anything could possibly go wrong .For working examples built on top of the skin: