Introduce sass skins - Githubissues

jplayer / jPlayer

jPlayer : HTML5 Audio & Video for jQuery

http://jplayer.org/

Other

4.6k stars 1.47k forks source link

Introduce sass skins #260

Closed nervo closed 9 years ago

thepag commented 9 years ago

Was just reviewing. Why has so much of the CSS file changed without actually changing? Tabs not spaces... And a lot of beautification? Rather, tabs changed to spaces. Due to the obfuscation of diff, the only changes I see as necessary are in the Gruntfile.js

nervo commented 9 years ago

That's just a first step :) The current sass code is just a copy/paste from css code with only a single variable to make things easier to adopt. Next, you can take advantage of sass syntax to optimize and simplify your code. The css file is the result of sass parsing, so, yes, the content is strictly the same, but indented automatically by sass. The same way, you could also have a minified version of your css. And yes, that's true, the big change appears in gruntfile, adding a new sass task.

thepag commented 9 years ago

Let me put it another way... I do not mind having sass setup, but why did it go and change the original CSS I wrote. Fair enough, it can go and create its own .scss files, but leave my files alone.

nervo commented 9 years ago

Well, because the main purpose of sass is to generate css files :) It's a win-win, you only maintain the sass files (a lot to gain), you expose them for those who use sass, and you let a css version for those stuck in the 90s :)

thepag commented 9 years ago

I thought I'd see what others thought first, but apparently no one cares, except you and now me.

I will be merging this pull request to give you the nod, but I will refactor it somewhat. The skin files will now be compiled by SASS into the /dist/ folder along with all the other distribution files. The old static css will now only appear in the volatile dist folder. No doubt a few other PRs will be affected by this, but many of those are old and no CLA so I'll have to implement them my way anyway.

thepag commented 9 years ago

Having just reviewed the changes you make and the way you did it, I would rather go about it a different way.

I want to commit each step:

make a /src/skin folder and move the /skin/blue.monday and /skin/pink.flag folders into /src/skin.
rename the blue.monday and pink.flag css files to scss
add the sass code to the scss files
add the sass grunt task to min.css into /dist/
add grunt task to copy the other blue.monday and pink.flag source css related files to dist

I'd then move what remains of the old /skin/ folder to the /lib/ folder where it belongs. Or something like that.

I would then update all the demos to point at the new place... Then probably push to 2.9.2 - and yes it counts as a restructure, but if I updated the major version every time I changed the structure, we would be in double figures by now.

nervo commented 9 years ago

Yeah man !

thepag commented 9 years ago

This is now in master and will go into the next 2.9.2 tag when I polish off the circle player changes. Cheers @nervo

nervo commented 9 years ago

Woops, i just realized sass files are not included in npm package :)

Would you ?