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

Sass improvements #1376

Merged
merged 2 commits into from Dec 1, 2018
Merged

Sass improvements #1376

merged 2 commits into from Dec 1, 2018

Conversation

@gpakosz
Copy link
Member

@gpakosz gpakosz commented Nov 25, 2018

This PR adds further improvements to the :sass and :sass_sourcemap filters.

Detailed description

  • :sass and :sass_sourcemap filters now require the :css_path option, which produces source maps where the optional "file" field has a correct value
  • the :sass filter now supports the generation of inline, base 64 encoded, source maps
  • Tests
  • Documentation

Related issues

nanoc/nanoc.app#227

@ddfreyne
Copy link
Member

@ddfreyne ddfreyne commented Nov 25, 2018

Requiring the :css_path option will be a backwards-incompatible change. Is there a way to have a default?

Loading

nanoc/lib/nanoc/filters/sass.rb Show resolved Hide resolved
Loading
nanoc/lib/nanoc/filters/sass.rb Show resolved Hide resolved
Loading
nanoc/lib/nanoc/filters/sass.rb Outdated Show resolved Hide resolved
Loading
@gpakosz
Copy link
Member Author

@gpakosz gpakosz commented Nov 25, 2018

It's indeed backward incompatible but it's the way it should have been in the first place 😢

Loading

@gpakosz
Copy link
Member Author

@gpakosz gpakosz commented Nov 26, 2018

Well I'll think about it, maybe I can do the following: if css_path is not given, use a dummy one then remove it from the generated sourcemap instead of letting incorrect information go through

Loading

@ddfreyne
Copy link
Member

@ddfreyne ddfreyne commented Nov 26, 2018

Alternatively, require css_path to be given only if sourcemap_path is set to :inline. What do you think?

Loading

@gpakosz
Copy link
Member Author

@gpakosz gpakosz commented Nov 26, 2018

In fact it has nothing to do with inline source maps. It's just Sass won't let you omit the optional "file" entry in the output source map. So the conservative strategy would be to give Sass a dummy value then change that after the source map has been produced

Loading

@gpakosz gpakosz force-pushed the sass-improvements branch from 87d6b7d to faefea0 Nov 26, 2018
@gpakosz
Copy link
Member Author

@gpakosz gpakosz commented Nov 26, 2018

There, now css_path is optional

Loading

@gpakosz gpakosz force-pushed the sass-improvements branch from faefea0 to b5fe9fa Dec 1, 2018
@ddfreyne ddfreyne merged commit 01f9076 into nanoc:master Dec 1, 2018
3 checks passed
Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants