Open chemoish opened 7 years ago
personally, I feel like moving away from sass (and some others have similar sentiments).
However, I think what should really happen is that these classes are created and then used directly rather than mixing them into the buttons. So, the play toggle should have a class of vjs-icon-play
and that the vjs-icon-play
won't be mixed in or extended into the vjs-play-toggle
class.
Having these live as separate classes that are used should make it easier to override, I'd hope.
I guess, I still really like sass, so I cannot really comment, but regardless of the processing, if you have each icon bake everything into it, then you will need to repeat code for every icon.
If you had a .vjs-icon
class, you can then bind font-family
one time and in one place. And that could be overwritten also in one place.
Just my two cents.
Feel free to close if there is already a solid direction you guys are headed in whatever repository.
Having a vjs-icon
and a vjs-icon-play
makes sense to me. That's what font-awesome does as well.
I had 2 classes in my first stab at this, but at this point I don't remember if I had a good reason for moving to one. I think all of this makes sense, though, I think moving the font-family declaration to a new class is reasonable.
personally, I feel like moving away from sass
I spent a while messing with CSSNext a few months ago and had a disastrous experience. 10/10 would not do again (for a while).
@mmcc but we need to move away from sass or at least fix our usage of sass because https://github.com/videojs/video.js/issues/3692. I don't think in videojs we're using anything specific of sass that isn't easily usable in postcss or just plain-old css. But that's a matter for another discussion :)
In similar spirit to https://github.com/videojs/video.js/issues/1880.
https://github.com/videojs/font/blob/master/scss/_icons.scss#L74
The code above specifies the
font-family
for every icon—making overriding difficult. My recommendation would be to separate that out and have a mixin be created to include thefont-family
and associated props only when needed.For controls such as play/pause or volume-0/volume-1/volume-2/volume-3, it really only requires the inclusion of the
font-family
on the control button. However, due to specificity rules I need to override everyone of them.Definitely not a 1:1 translation, but this is the way I manage custom icons.
I then have the flexibility to only include
icon-font
when necessary. Additionally, I need to add the appropriatecontent: $icon-{name}
.