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

Introduce sass skins #260

Closed
wants to merge 1 commit into from
Closed

Introduce sass skins #260

wants to merge 1 commit into from

Conversation

nervo
Copy link
Contributor

@nervo nervo commented Nov 18, 2014

No description provided.

@thepag
Copy link
Contributor

thepag commented Nov 19, 2014

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
Copy link
Contributor Author

nervo commented Nov 19, 2014

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
Copy link
Contributor

thepag commented Nov 19, 2014

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
Copy link
Contributor Author

nervo commented Nov 19, 2014

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
Copy link
Contributor

thepag commented Dec 14, 2014

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
Copy link
Contributor

thepag commented Dec 14, 2014

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
Copy link
Contributor Author

nervo commented Dec 14, 2014

Yeah man !

thepag added a commit that referenced this pull request Dec 14, 2014
thepag added a commit that referenced this pull request Dec 14, 2014
thepag added a commit that referenced this pull request Dec 14, 2014
@thepag
Copy link
Contributor

thepag commented Dec 14, 2014

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

@thepag thepag closed this Dec 14, 2014
@nervo
Copy link
Contributor Author

nervo commented Jun 15, 2015

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

Would you ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants