chore: better sass dependency management #11986
Conversation
|
Kudos, SonarCloud Quality Gate passed!
|
There was a problem hiding this comment.
This is a big breaking change regarding the changes. Also this will break the Sass version as users have to run npm i now. There is a good reason why the vendor folder is in the published packages.
I would close this PR and use the correct way and improve it in small steps without such changes.
|
OK. There went quite a few hours of work... oh well. I will close the other one as well. |
| SASS_DOC_PATHS: [ | ||
| 'scss', | ||
| 'node_modules/motion-ui/src', | ||
| 'node_modules/foundation-docs/scss' | ||
| 'node_modules/foundation-docs/scss', | ||
| 'node_modules/sassy-lists/stylesheets' |
| @@ -66,6 +56,7 @@ module.exports = { | |||
| ], | |||
|
|
|||
| DIST_FILES: [ | |||
| './_build/assets/scss/foundation.scss', | |||
There was a problem hiding this comment.
Makes not much sense, this is not a dist file.
| // Creates a Sass file from the module/variable list and creates foundation.css and foundation.min.css | ||
| gulp.task('customizer:sass', gulp.series('customizer:loadConfig', 'customizer:prepareSassDeps', function() { | ||
| gulp.task('customizer:sass', gulp.series('customizer:loadConfig', function() { |
There was a problem hiding this comment.
Removing things to fix them is not the correct approach.
| includePaths: [ | ||
| 'scss', | ||
| 'node_modules/motion-ui/src' | ||
| ] | ||
| })) | ||
| }))) |
There was a problem hiding this comment.
Badly formatted and hard to read.
| @@ -22,7 +22,7 @@ var NEXT_VERSION; | |||
| gulp.task('deploy', gulp.series('deploy:prompt', 'deploy:version', 'deploy:dist', 'deploy:plugins', 'deploy:settings', 'deploy:commit', 'deploy:templates')); | |||
|
|
|||
| gulp.task('deploy:prep', gulp.series('deploy:prompt', 'deploy:version', 'deploy:dist', 'deploy:plugins', 'deploy:settings')); | |||
| gulp.task('deploy:dist', gulp.series('sass:foundation', 'javascript:foundation', 'deploy:dist:files', 'deploy:dist:bundles')); | |||
| gulp.task('deploy:dist', gulp.series('sass:foundationCSS', 'javascript:foundation', 'deploy:dist:files', 'deploy:dist:bundles')); | |||
There was a problem hiding this comment.
Renaming tasks will break implementations and docs.
| @@ -1,45 +1,46 @@ | |||
| 'use strict'; | |||
There was a problem hiding this comment.
Disabling strict mode allows more broken JS.
| gulp.task('sass:foundationSCSS', function() { | ||
| return gulp.src(['assets/foundation.scss']) | ||
| .pipe(gss(eyeglass())) | ||
| .pipe(rename('foundation.scss')) |
There was a problem hiding this comment.
Makes it all unnecessary complicated.
| @@ -159,5 +161,8 @@ | |||
| "commitizen": { | |||
| "path": "./node_modules/cz-conventional-changelog" | |||
| } | |||
| }, | |||
| "dependencies": { | |||
| "normalize.css": "^8.0.1" | |||
There was a problem hiding this comment.
This should not be there.
| @import '../_vendor/sassy-lists/stylesheets/functions/remove'; | ||
| @import '../_vendor/sassy-lists/stylesheets/functions/replace'; | ||
| @import '../_vendor/sassy-lists/stylesheets/functions/to-list'; | ||
| @import '../node_modules/sassy-lists/stylesheets/SassyLists'; |
There was a problem hiding this comment.
Will not work in the pure scss version.
| } | ||
|
|
||
| // 1. Correct the inability to style clickable types in iOS and Safari. |
There was a problem hiding this comment.
Normally there should be still the comments.
Please use the original file, not the production file.
https://github.com/necolas/normalize.css/blob/8.0.1/normalize.css#L304
Description
This is another attempt at #9542
I could not get sass:foundationSCSS to work with gulp-shopify-sass and eyeglass. The code is still in there but I removed the taks from being ran. Maybe we can figure it out one day.
The sassy-lists libraries are imported from node_modules now.
Normalize.css is still manually added into vendor/normalize. The reason that this needs to be there is that would need to be imported from within a mixin. Sass does not allow this yet. There for it needs to be made into its own mixin. A better way would be to simply have a separate import command for this. However, this would break existing implementations and we do not want to do that. The normalize.css does not change frequently at all. So this is not too big of a deal right now. Its definitely something that could be fixed in F7 though.
Types of changes