Skip to content
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

All about the styles: Stylesheet organization / Sass migration / third party themes. #1492

Closed
mmcc opened this issue Sep 8, 2014 · 12 comments
Labels
enhancement needs: discussion Needs a broader discussion
Milestone

Comments

@mmcc
Copy link
Member

mmcc commented Sep 8, 2014

We've been talking about doing this for quite some time, but I think it's finally time to actually start a conversation. We've always stuck to a single Less file for three main reasons:

  • Simplicity (reduce the barrier to entry for folks new to preprocessors)
  • Designer (one file to load into the designer and edit)
  • Because, because.

As the time is getting nigh for version 5, one of the big things we'd like to tackle is standardizing practices for creating third party themes. Before we can start talking about third party themes, I think we need to lead by example with our default theme.

  • Sass vs Less? To be quite honest, a big reason for picking Less in the first place was browser support for the preprocessor (so we could have the designer, for instance). That's definitely not an issue anymore and it seems that Sass is winning the war for designer's hearts (and I personally prefer it, which is obviously the most important).
  • Separate repo or same? Should the default theme have its own home or still live within the core repo?
  • Style guide? I've been stealing using a lot of @fat's high level Less practices. Is that ok with us? Do we want to use more? Less?
  • Directory structure - How do we want to organize themes? What should the delivery mechanism be?

I have my own opinions on a lot of these things, but I'm going to keep quiet for now (other than Sass > Less :trollface:). Over the weekend I did a fast and dirty conversion to Sass while also breaking the files up a bit into a way that I thought made sense. It's in my personal repos right now and will stay there until we decide otherwise to avoid confusion, but opinions welcome: mmcc/videojs-default-skin. I went ahead and made a codepen as well that shows how this could all potentially work together.

@albell
Copy link
Contributor

albell commented Sep 8, 2014

Sass vs Less?

You know the answer!

Separate repo or same?

Bundle the default skin together for convenience.

Style guide? I've been stealing using a lot of @fat's high level Less practices. Is that ok with us? Do we want to use more? Less?

@fat best practices LGTM. In particular I have some ideas about reducing nesting and specificity. The one beef I've got with that doc is the idea of handling all vendor prefixes with mixins, which I really object to. I think Autoprefixer is the way to go here. The whole idea of having to learning a new syntax abstraction to handle backwards compatibility is just wrong in principle. Whenever possible, the spec should be the syntax--it's enough to learn as is. Authors should be able to write plain CSS as much as possible using the spec, and have prefixes added as appropriate based on the depth of backwards compatibility specified in an editable JSON config tucked away in src/css. Preprocessors should reduce cognitive load, not add to it. This is also more future proof, because as vendor prefixes begin to fade into historical memory, this can continue to be the way to go. I honestly think using mixins for prefixing will soon seem archaic. I could be wrong, but I imagine vjs' Sass really only needing variables. I can open a separate issue for Autoprefixer if there's interest in this.

Directory structure - How do we want to organize themes? What should the delivery mechanism be?

Redundancy in the dist is ok if there's clear file naming, e.g.:

vjs-core.css
vjs-default-skin.css
vjs-core+default-skin.min.css

my 2 cents.

@mmcc
Copy link
Member Author

mmcc commented Sep 8, 2014

Bundle the default skin together for convenience.

Well to be clear, the default skin would always be bundled with Video.js from a dist perspective, it's just whether or not to bundle the source files as well.

@gkatsev
Copy link
Member

gkatsev commented Sep 8, 2014

Yes, the css source files should be in the main repo. Any alternate skins or alternate formats of the default skin should be in separate repos.

@nervo
Copy link

nervo commented Sep 30, 2014

My two cents for a clean sass introduction :

  • Prefix variables ( something like $vjs_background_color)
  • Separate variables AND mixins files (_variables.scss & _mixins.scss)
  • Have a dedicated style file (something like _style.scss)
  • A global non-underscored file, which includes _variables, _mixins and _style

@albell
Copy link
Contributor

albell commented Sep 30, 2014

@nervo LGTM. I would only add that the mixin file should be kept lean, and not used for handling prefixing. One of the beauties of autoprefixer is that dropping CSS support for a browser (IE8 someday!) is as simple as deleting a few characters from the gruntfile.

@gkatsev
Copy link
Member

gkatsev commented Sep 30, 2014

prefixing would be nice to be handled automatically via autoprefixer or similar project.

@nervo
Copy link

nervo commented Oct 1, 2014

👍 We already use autoprefixer with success

@mmcc
Copy link
Member Author

mmcc commented Oct 1, 2014

@nervo did you take a look at the example repo? Everything except for the variable prefixing are already implemented there.

@nervo
Copy link

nervo commented Oct 1, 2014

Oh, sorry, i misunderstood code was already there, nice job.
Btw, i really think variables prefixing is a must have; i would appreciate to embed videojs sass style, mixed with other projects, without naming collisions.

@mmcc
Copy link
Member Author

mmcc commented Oct 1, 2014

I think that's a fair argument, will implement.

@heff heff added this to the v5.0.0 milestone Apr 28, 2015
@gkatsev
Copy link
Member

gkatsev commented May 1, 2015

I think this has been decided on. Can it be closed? @mmcc @heff.

@mmcc
Copy link
Member Author

mmcc commented May 1, 2015

Yep, with base theme merged this is done. 


Sent from mobile

On Fri, May 1, 2015 at 2:16 PM, Gary Katsevman [email protected]
wrote:

I think this has been decided on. Can it be closed? @mmcc @heff.

Reply to this email directly or view it on GitHub:
#1492 (comment)

@heff heff closed this as completed May 1, 2015
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement needs: discussion Needs a broader discussion
Projects
None yet
Development

No branches or pull requests

5 participants