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

node_modules support for sass imports #211

Merged
merged 9 commits into from
Nov 22, 2016
Merged

node_modules support for sass imports #211

merged 9 commits into from
Nov 22, 2016

Conversation

giladgray
Copy link
Contributor

@giladgray giladgray commented Nov 18, 2016

PR checklist

What changes did you make?

use node-sass-package-importer to support package names in sass imports instead of relative paths, which break when trying to compile sass APIs outside the monorepo. package paths must begin with the ~ character, like webpack sass-loader: @import "~@blueprintjs/core/dist/blueprint.css".

update relevant @import paths.

Gilad Gray added 2 commits November 18, 2016 12:28
to support package names in sass imports
instead of relative paths, which break when trying to compile sass APIs outside the monorepo.
@themadcreator
Copy link
Contributor

I'm excited for this! just waiting for previews to load before +2'ing

Copy link
Contributor

@themadcreator themadcreator left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Icon fonts no longer loading:

image

@giladgray
Copy link
Contributor Author

yeah something broke with your postcss-copy-assets task...

this fork does not inline .css files, instead resolving them to relative paths.
@blueprint-bot
Copy link

[WIP] use @giladgray/node-sass-import instead

Preview: docs | landing | table Coverage: core | datetime

@blueprint-bot
Copy link

fix package.json

Preview: docs | landing | table Coverage: core | datetime

@giladgray
Copy link
Contributor Author

this has been fixed! @themadcreator you around for a re-review?

@@ -45,7 +45,9 @@ module.exports = (gulp, plugins, blueprint) => {
));

blueprint.task("sass", "compile", ["icons", "sass-variables"], (project, isDevMode) => {
const sassCompiler = plugins.sass();
const sassCompiler = plugins.sass({
importer: require("node-sass-package-importer")({ cwd: project.cwd }),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please put the require with all the other imports. no reason to do it inline here.

@adidahiya
Copy link
Contributor

lgtm aside from the one comment

@giladgray giladgray dismissed themadcreator’s stale review November 22, 2016 21:27

issue resolved but bill is on vacation

@blueprint-bot
Copy link

move require to the top

Preview: docs | landing | table Coverage: core | datetime

@giladgray giladgray merged commit 39f3cfc into master Nov 22, 2016
@giladgray giladgray deleted the gg/sass-imports branch November 22, 2016 21:37
@giladgray giladgray mentioned this pull request Nov 22, 2016
greglo pushed a commit to greglo/blueprint that referenced this pull request Dec 12, 2016
* configure node-sass-import

to support package names in sass imports
instead of relative paths, which break when trying to compile sass APIs outside the monorepo.

* update `@import` paths to use `~` prefix

* use node-sass-package-importer and "~package" syntax
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants