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

Fix #9412 - Import normalize-scss with its full path #9449

Closed

Conversation

ncoden
Copy link
Contributor

@ncoden ncoden commented Dec 5, 2016

Import normalize-scss with its full path instead of including the assets path in gulp-sass.

Fix #9412

Import normalize-scss with its full path instead of including the
assets path in gulp-sass.

Fix foundation#9412
@ncoden
Copy link
Contributor Author

ncoden commented Dec 5, 2016

Poke @kball foundation@6.3.0-rc1 is broken. This is a hotfix.

@oxyc
Copy link

oxyc commented Dec 5, 2016

This doesn't work with foundation as a NPM dependency as normalize-scss will be in the root node_modules/. I can fix it by requiring an older version of normalize-scss in my project root and thus installing foundations version within it's own node_modules/ directory.

@kball
Copy link
Contributor

kball commented Dec 5, 2016

so... I'm a little confused about this because

  1. the current build is working for me... what am I doing differently?
  2. this seems to overly subscribe how your dependencies are located - what happens if someone uses a custom node modules path, or is using bower instead of npm? Paths should be a part of the build, not the src

@oxyc
Copy link

oxyc commented Dec 5, 2016

https://github.com/npm/npm/blob/ff47bc138e877835f1c0f419fea5f5672110678a/CHANGELOG.md#flat-flat-flat

So the default behaviour for foundation-sites installed as a dependency would not work unless you introduced a version conflict for normalize-scss.

Partially fix normalize-scss import where foundation-sites was imported
from npm.
@ncoden ncoden force-pushed the fix/9412-normalize-scss-import branch from ad8dbb3 to 8be4595 Compare December 5, 2016 18:43
@ncoden
Copy link
Contributor Author

ncoden commented Dec 6, 2016

When Foundation is used in a Sass project via NPM or Bower, it is by importing /scss/foundation.scss, with the project build process. But /scss/foundation.scss needs its own dependencies, needs to know their path, and thus they can't be imported via NPM or Bower as real dependencies.

We have to install our Sass dependencies (like normalize-scss) via NPM only as a dev dependencies, then copy them to /scss/_vendor in the sass gulp build task. By example for normalize, it can be imported in /scss/foundation.scss with:

@import "_vendor/normalize-scss/sass/normalize";

When the package is published, the sass gulp build task is called too, and Sass dependencies are still available in /scss/_vendor.

@hugogiraudel @kball What do you think ?

ncoden added a commit to ncoden/foundation-sites that referenced this pull request Dec 6, 2016
Previous PR: foundation#9449

Changes:
- Revert foundation#9430 - Make
`normalize-scss` a dev dependency.
- Copy Sass dependencies to `/_vendor/scss` and import them in Sass
from there.

Why not importing the Sass dependencies as real dependencies and from
`node_modules`: we cannot assume the path to our real dependencies. Old
npm versions store them in our `node_modules` folder, while newer
versions store them in the `node_modules` folder of the root project.
@ncoden
Copy link
Contributor Author

ncoden commented Dec 6, 2016

I close this PR. See: #9458

@ncoden ncoden closed this Dec 6, 2016
@KittyGiraudel
Copy link
Contributor

That seems like a valid idea.

@ncoden
Copy link
Contributor Author

ncoden commented Dec 6, 2016

@hugogiraudel Did you seen the PR ?

@ncoden ncoden deleted the fix/9412-normalize-scss-import branch December 7, 2016 06:52
@ncoden ncoden restored the fix/9412-normalize-scss-import branch January 6, 2017 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants