thoughtbot / carnival

An unobtrusive, developer-friendly way to add comments
MIT License
499 stars 30 forks source link

Attempt to add Sass support #219

Closed pbrisbin closed 9 years ago

pbrisbin commented 9 years ago

Theory:

I have no idea what I'm doing and would love a Designer to try this out / see if it's what's needed to use our normal work-flow.

cpytel commented 9 years ago

Can we remove compass from this and just have straight sass?

pbrisbin commented 9 years ago

I have no idea. Is there a sass watch? Does it respect config.rb? I had to install compass to get compass init to generate it. The usage for sass made me think it always requires INPUT/OUTPUT arguments.

jferris commented 9 years ago

You can do sass --watch source_dir:output_dir.

pbrisbin commented 9 years ago

I'm trying to determine if there's a way to get sass --watch to do the after_save, otherwise they'll have to manually touch Settings/StaticFiles.hs on changes -- or install another tool.

pbrisbin commented 9 years ago

OK, so that's not so bad. You only need to touch when you add a new CSS file, which should be infrequent. Changes take effect right away.

pbrisbin commented 9 years ago

Ping @smharley @tysongach

Also: I assume we'll want to move all the *.lucius files into sass/. I could do that here or after.

tysongach commented 9 years ago

This is super similar to what we recently did on a Shopify project. Shopify actually compiles Sass on their end, but they don’t support Sass @import’s, leaving the option to use Bourbon out.

So we instead made a sass directory, put Bourbon and all of our project partials in there, then used Gulp to concatenate (but not compile down to CSS) all the Sass partials into a single Sass file placed in the proper Shopify location. We also committed both partialed and concatenated Sass.

Without trying it, this sounds like a decent workflow. :+1:

Curious, what’s the purpose of sass/print.scss? Are there print styles in here, maybe for printing article comments?

pbrisbin commented 9 years ago

Curious, what’s the purpose of sass/print.scss? Are there print styles in here, maybe for printing article comments?

It was added by compass init. I'll take it out.

tysongach commented 9 years ago

Cool. Yeah definitely no Compass needed.

This is super great, @pbrisbin!

pbrisbin commented 9 years ago

OK, I think this is ready for final review.

The first commit is good, it just adds the Sass machinery. The second commit is required to get our Sass stuff and Bootstrap playing nicely together again. The third commit needs some additional designer review:

With this in place, I'm pretty sure you could use the same approach, @tysongach, just dump bourbon partials under sass/ (maybe sass/vendor?) and @import them.

smharley commented 9 years ago

I'm good with how the files are split up, for now. It'll probably change a bit if we get bitters in. Markup also looks good for the time being, though it might make sense to have the delete button styles be a little more reusable in the future.

In the past, I've git ignored the sass .map files, but I'm not sure if that's necessary. I pulled your branch and checked, everything seems to look right!

pbrisbin commented 9 years ago

Perfect. I think I'll include the .map files if only so that we don't see 404's when visiting production/staging attempts to load them.