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

Add support for Sass and SCSS. #1932

Merged
merged 14 commits into from Jan 24, 2014

Conversation

Projects
None yet
10 participants
@parkr
Member

parkr commented Jan 12, 2014

This pull request adds support for Sass to Jekyll proper.

  • Implement converters
  • Write tests that test converter functionality

The question remains: Should we enforce this or ship jekyll-sass that adds support for this externally?

@ghost ghost assigned mattr- Jan 12, 2014

$font-stack: Helvetica, sans-serif
body
font-family: $font-stack
font-color: fuschia

This comment has been minimized.

@doktorbro

doktorbro Jan 12, 2014

Member

You mean fuchsia, not?

This comment has been minimized.

@parkr

parkr Jan 12, 2014

Member

Yes! It was close to 2:30 am when I wrote this code, so my apologies for the spelling error. It doesn't change the test that much, though, as Sass doesn't care! :)

@mattr-

This comment has been minimized.

Member

mattr- commented Jan 12, 2014

oooooh, shiny!

My thoughts are to ship it with core unless we can ship it as a gem and still use it with GitHub Pages.

@mattr-

This comment has been minimized.

Member

mattr- commented Jan 12, 2014

or i suppose we could ship it with core now and then extract it to a gem later and include it as a dependency by default.

@doktorbro

This comment has been minimized.

Member

doktorbro commented Jan 12, 2014

@parkr

  1. How to set output style in _config.yml?
  2. Is the watch option from Jekyll passed to Saas?
@parkr

This comment has been minimized.

Member

parkr commented Jan 12, 2014

@penibelst You bring up good points.

  1. The output style should be configurable via _config.yml.
  2. The watch option is not passed to Sass, because this process runs every time Jekyll is built, i.e. Jekyll does the watching for Sass.

@ghost ghost assigned parkr Jan 12, 2014

@parkr

This comment has been minimized.

Member

parkr commented Jan 12, 2014

or i suppose we could ship it with core now and then extract it to a gem later and include it as a dependency by default.

It might provide a great example for future converter plugin users to see how it's put together. If it's shipped with core, it's enabled by default. If it's shipped separately, one will have to enable it manually via the gems option in the site config.

@parkr

This comment has been minimized.

Member

parkr commented Jan 12, 2014

@mattr- I think this is ready to go. How do my tests look?

@parkr

This comment has been minimized.

Member

parkr commented Jan 12, 2014

Also, in terms of docs, I think we should create a new page that talks about both Sass and CoffeeScript support & configuration. I'll add CS stuff in a second PR to support that. :)

@doktorbro

This comment has been minimized.

Member

doktorbro commented Jan 13, 2014

@parkr If my file /css/screen.scss contains

@import "normalize";
@import "grid";

will Sass look for /_sass/_normalize.scss and /_sass/_grid.scss?

".css"
end
def user_sass_configs

This comment has been minimized.

@mattr-

mattr- Jan 13, 2014

Member

jekyll_sass_config or just sass_config perhaps? I'm much more in favor of a singular name here config instead of configs since we only have one. 😃

This comment has been minimized.

@parkr

parkr Jan 24, 2014

Member

Strictly speaking they're "configuration settings" or "configuration options", which is plural, though the resulting Hash is only one element (which contains multiple sub-elements, namely keys and values of our configuration names and values).

@config["sass"] || {}
end
def sass_build_configuration_options(overrides)

This comment has been minimized.

@mattr-

mattr- Jan 13, 2014

Member

you spell out 'configuration' here but not above. Can we be consistent here?

This comment has been minimized.

@parkr

parkr Jan 24, 2014

Member

I'll spell out configuration for all of them

user_sass_configs.deep_merge(overrides).symbolize_keys
end
def syntax_type_of_content(content)

This comment has been minimized.

@mattr-

mattr- Jan 13, 2014

Member

Maybe this is better?

def type_of_content(content)
  return :scss if content.match(/;|{/)
  :sass
end

This comment has been minimized.

@parkr

parkr Jan 24, 2014

Member

Why is that better? Other than fewer lines, I don't see the advantage.

end
def sass_dir
user_sass_configs["sass_dir"] || "_sass"

This comment has been minimized.

@mattr-

mattr- Jan 13, 2014

Member

user_sass_configs.fetch("sass_dir", "_sass")

Either one works. Just providing options at this point. 😃

This comment has been minimized.

@parkr

parkr Jan 13, 2014

Member

The one you suggest will return nil if sass_dir is set to nil. I want any falsey value of sass_dir to be overridden by _sass... and also "", so I'll need to change this anyway.

This comment has been minimized.

@mattr-

mattr- Jan 13, 2014

Member
return "_sass" if user_sass_configs["sass_dir"].to_s.empty?
user_sass_configs["sass_dir"]
context "determining the output file extension" do
should "output .css file extension" do
assert_equal ".css", converter.output_ext(".sass")

This comment has been minimized.

@mattr-

mattr- Jan 13, 2014

Member

because SASS supports both SCSS and SASS style content, you might be able to better articulate that the converter doesn't care about the content type with

assert_equal ".css", converter.output_ext(".always-css")

in this test. I only point this out because seeing this here made me go back and see if there was a conditional in output_ext 😃

This comment has been minimized.

@parkr

parkr Jan 13, 2014

Member

👍

context "when building configurations" do
should "not allow caching in safe mode" do
verter = converter
verter.instance_variable_get(:@config)["safe"] = true

This comment has been minimized.

@mattr-

mattr- Jan 13, 2014

Member

converter({"safe" => false}) instead of this ickiness?

This comment has been minimized.

@parkr

parkr Jan 13, 2014

Member

Yeah, but then I have to do {"sass" => whatever} everywhere else :(

This comment has been minimized.

@mattr-

mattr- Jan 13, 2014

Member

ah, right. misread that part :-(

@parkr

This comment has been minimized.

Member

parkr commented on test/test_sass.rb in 824d9f6 Jan 16, 2014

Maybe "always outputs the .css file extension"?

This comment has been minimized.

Member

mattr- replied Jan 16, 2014

Yes! Uno momento.

@mattr-

This comment has been minimized.

Member

mattr- commented Jan 16, 2014

Whoops. Don't know how I got two commits with the exact same message in there.

@doktorbro

This comment has been minimized.

Member

doktorbro commented Jan 18, 2014

@mscharley, should partials live in the _sass folder? See #1932 (comment)

@mscharley

This comment has been minimized.

Contributor

mscharley commented Jan 18, 2014

@penibelst That's actually a really good question and potential solution. Having the imports in _sass would automatically exclude them from Jekyll, but would play hell with IDE's not being able to trace imports properly. Looking at the code, it's possible it does, but I'm not 100% familiar enough with the Sass codebase to make a call one way or the other based on the current code in this PR.

That said, if it doesn't, Sass does support include paths so that could possibly be another way to get around this in Jekyll.

Or just do it the old fashioned way and @import '../_sass/normalize' will of course still work as well and not have the IDE issues. Still, it's not as clean to read.

For what it's worth, my 2c is that it's really nice to have everything in the one folder. But any Sass support being in jekyll core is a good thing.

@parkr

This comment has been minimized.

Member

parkr commented Jan 24, 2014

@mscharley You can set the sass_dir in your configs, which defaults to _sass inside source.

@mattr-

This comment has been minimized.

Member

mattr- commented on c9a3c40 Jan 24, 2014

❤️

mattr- added a commit that referenced this pull request Jan 24, 2014

@mattr- mattr- merged commit 5b7a53b into master Jan 24, 2014

1 check passed

default The Travis CI build passed
Details

mattr- added a commit that referenced this pull request Jan 24, 2014

@parkr parkr deleted the sass branch Jan 25, 2014

@Wolfr

This comment has been minimized.

Wolfr commented Jan 25, 2014

Just want to weigh in here and give my opinion as a heavy Jekyll user.

Integrating Sass compilation in Jekyll is an interesting proposition (especially if you take light gh-pages usage into account) but I don't think it belongs in core.

Jekyll is about generating static websites in the easiest way possible, with awesome text manipulation.

We have tools like Grunt and Gulp to automate tasks via Javascript like Sass, Coffeescript, linting, etc.

I feel that this idea duplicates a bit of the functionality provided by Grunt/gulp and will make Jekyll harder to maintain. I feel Jekyll 2.0 should focus on the core of what Jekyll does best i.e. converting text into static websites (the HTML part). I have seen many cool additions to Jekyll like parametrized includes and _data which I have used to great effect in my everyday work.

Jekyll is the part that compiles your Liquid templates to HTML. The Sass compiler is the part that compiles your SCSS into CSS. Why integrate the Sass compiler in Jekyll now?

Will I have to configure Jekyll to stop compiling SCSS in _config.yml because I actually want to use the C libsass implementation via Grunt? That would feel awkward.

Will we also integrate Coffeescript compilation in Jekyll? Where does it end?

In no way do I want to disrespect the work that has been done here and I'm sure there are plenty of use cases where you want Sass directly in the Jekyll compiler for "small blog/website" work where you don't want to bother with tools like Grunt.

TL;DR: :+1 for a separate jekyll-sass.

@doktorbro

This comment has been minimized.

Member

doktorbro commented Jan 25, 2014

@Wolfr Sass is the best friend of Css. Css is the best friend of Html. Jekyll brings the things together by adding 130 lines of code + docs. This easy addition feels natural to me. Should I learn Grunt/gulp just to write maintainable Css?

@Wolfr

This comment has been minimized.

Wolfr commented Jan 25, 2014

@penibelst No you should just run 2 compilers, one for Jekyll and one for SCSS if you want to keep things simple.

@mscharley

This comment has been minimized.

Contributor

mscharley commented Jan 25, 2014

As much as I hate to, I have to agree with @Wolfr for two reasons.

  1. Why Sass? If the goal is maintainable CSS, why not LESS? Or any of the other options? Will it stop with CSS, or will it include maintainable js (coffeescript).
  2. How does this inclusion play with plugins that go further than just Sass? For example, jekyll-compass. Compass is so much more than just Sass, who's parser will win out? Or even worse, what about the person who wants to integrate Compass manually but has to fight against Jekyll parsing their sass?
@mscharley

This comment has been minimized.

Contributor

mscharley commented Jan 25, 2014

For what it's worth, I'll repeat what I said before: This already exists as a plugin, it's jekyll-compass. Adding it to jekyll is as easy as installing the gem and adding it to the list of gems plugins in your website. Sensible defaults prevail, fully customisable if you don't like them.

The one benefit I see to it being in core is GH Pages. But mattr- already mentioned that they could load gem plugins, so that's potentially a null issue. And I think this is the better option long term anyway; as more of these gem plugins get created, Pages could pull in more and more safe plugins to extend what they allow.

@cameronroe

This comment has been minimized.

cameronroe commented Jan 25, 2014

I also have to agree with @Wolfr and @mscharley. Sass is a popular language, but LESS users may have a difficult time integrating. As much as I love Sass, I would much rather use Grunt as it has much more flexibility.

@parkr

This comment has been minimized.

Member

parkr commented Jan 25, 2014

Thanks for the input, guys.

As much as I love Sass, I would much rather use Grunt as it has much more flexibility

We're not forcing anyone to use Jekyll's Sass converter. It's just an option so you don't have to pre-compile before pushing up to GHP or your own Jekyll host. It removes the barrier of "yet another thing to learn" for newcomers.

This already exists as a plugin, it's jekyll-compass

A couple things about this:

  1. I appreciate that plugins exist for this functionality already, but we have no power over plugins on GHP as Jekyll maintainers, so if we want to see something on GHP, it makes more sense to bundle it in with Jekyll core for now.
  2. If we were to create a "jekyll-sass" plugin, we'd still have to specify it as a runtime dependency, and we would get all issues/bugs for it filed here anyway.
  3. We didn't want Compass, we wanted Sass.

Will we also integrate Coffeescript compilation in Jekyll?

Yep, that's in the pipeline.

No you should just run 2 compilers, one for Jekyll and one for SCSS if you want to keep things simple

This is what we want to avoid. Part of what I like about Jekyll right now is that I don't need to start up a markdown converter, and boot my own web server, etc. It's all in one command: jekyll serve or (if you don't need a web server) jekyll build. Jekyll needs to absorb that complexity for the user.

Sass is a popular language, but LESS users may have a difficult time integrating

Sass seems far more popular, and is integrated into Rails and other common frameworks already. It was an easy choice. No one has voiced a wish for LESS or Stylus support. At that time, we can evaluate the merits of bringing that in.

@robwierzbowski

This comment has been minimized.

Contributor

robwierzbowski commented Jan 28, 2014

Seems like some people already dropped in with opinions similar to mine.

I initially thought this was a dangerous precedent, but after taking a look at the code I feel better. Very simple, just like a markdown parser, and well contained if maintainers decide to move it into a separate gem in the future.

One of the things I value about Jekyll is its simplicity and focus. It's a data and markdown parser that outputs HTML. Features like this that bring it towards an all-in-one-command blogging package seems to violate the "do one thing and do it well" principle of modular software design. Jekyll is a part of a flat file application builder, just like Sass or any other language preprocessor — the more focused and modular the better. OK, it does run a simple development server too, but as the HTML compiler that makes sense to me 😄.

No you should just run 2 compilers, one for Jekyll and one for SCSS if you want to keep things simple.
This is what we want to avoid.

I don't see why this is something to avoid. Simple projects like guard-sass coupled with guard-jekyll have this problem solved. Two terminal windows have this problem solved. I think those in this thread would feel better if the maintainers instead said "we'd like to provide some options".

It's a pretty small code addition overall, so I'm not really worried as long as it isn't on by default. Maybe in the future it can be refactored into a Github-safe gem that integrates Sass and Jekyll together.

@parkr

This comment has been minimized.

Member

parkr commented Jan 28, 2014

It's a pretty small code addition overall, so I'm not really worried as long as it isn't on by default

At the moment, it's on by default, just like the markdown converters. If there's a .sass or .scss file in a directory somewhere in your site with YAML front-matter, it'll convert.

@robwierzbowski

This comment has been minimized.

Contributor

robwierzbowski commented Jan 28, 2014

with yank front matter

That's cool too, adding the front matter is a concious opt in. I wouldn't want Jekyll to suddenly hijack my workflows.

@mscharley

This comment has been minimized.

Contributor

mscharley commented Jan 28, 2014

Yeah, the "with YAML front-matter" part is the bit I was missing as well.
This conditional solves all my compatibility concerns.

Though, I do wonder, does that mean that Sass parsed this way will support
Liquid as well? That could lead to some... "interesting" uses by people.

@parkr

This comment has been minimized.

Member

parkr commented Jan 28, 2014

does that mean that Sass parsed this way will support Liquid as well?

Yes, you may place Liquid in your Sass files which Jekyll parses. This could be very useful for theming using config files.

@budparr

This comment has been minimized.

Contributor

budparr commented Jan 28, 2014

Yes, you may place Liquid in your Sass files which Jekyll parses.

That alone makes this an excellent idea - I'm using _data files for theming parts of a site, and right now I'm pulling those into a template, which eliminates the use of Sass variables.

def sass_dir_relative_to_site_source
File.join(
@config["source"],
File.expand_path(sass_dir, "/") # FIXME: Not windows-compatible

This comment has been minimized.

@shiye515

shiye515 Apr 16, 2014

按照官方网站的介绍试了半天,结果在这发现不支持windows。。。:(

This comment has been minimized.

@parkr

This comment has been minimized.

@shiye515

shiye515 Apr 16, 2014

thanks 👍 :)

@jekyll jekyll locked and limited conversation to collaborators Feb 27, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.