Jenessawhite/fix sass #35
Conversation
package.json
Outdated
| @@ -4,16 +4,21 @@ | |||
| "private": true, | |||
| "dependencies": { | |||
| "@blueprintjs/core": "^2.2.1", | |||
| "node-sass": "^4.9.0", | |||
| "node-sass-chokidar": "1.3.0", | |||
There was a problem hiding this comment.
I think I owe some context on why we weren't using node-sass-chokidar to begin with.
In 8876362, I upgraded the version of react-scripts we use so that we can take advantage of improvements in v2 of that package.
The full list of improvements is described in this GH issue, but the main reason we sprung for this is for the upgraded webpack configuration.
However, because I made a mistake in pushing that code directly to master (I intended to branch, but my night-brain didn't comply), I didn't sufficiently explain what was happening. I can see why one would go this route without that context.
The stable version of react-scripts (which is currently referenced in the README for the master branch of create-react-app) doesn't include any of these preprocessors as peer dependencies, which is why they recommend the use of a separate process to build styles than the rest of your app. The version we're using as of 8876362 does include those preprocessors, which is why we don't need to install node-sass-chokidar or add these separate build scripts.
That said, I know you mentioned you were running into issues when trying to use this react-scripts@2 as-is. A few things I'd recommend checking out after reverting the addition of node-sass-chokidar, but keeping the SCSS additions intact:
- Clear your browser cache
- Remove and reinstall all packages, then run
yarn start
If this doesn't cut it, we might want to consider removing the service worker that CRA adds by default. As this is essentially a static site, having the service worker present seems unnecessary.
Let me know if I can add more detail around the above. I know this is kind of a wall-of-text, trying to get all this on paper before standup :)
There was a problem hiding this comment.
Per our internal discussion, I'm no longer of the opinion that my above message tracks the right way to go. Let's ship this! Thanks, @jenessawhite!
| // Just copy a variable from src/sass/bootstrap4/_variables.scss, paste it here and edit the value. | ||
|
|
||
| // COLORS | ||
| $pink: #c9297b; |
There was a problem hiding this comment.
Love the vars! So good!
| @import "common"; | ||
| @import "header"; | ||
| @import "sections"; | ||
| @import '//cdn-images.mailchimp.com/embedcode/horizontal-slim-10_7.css'; |
There was a problem hiding this comment.
This isn't super germane to this PR, but this mailchimp stylesheet feels like it should be closer to the component where we're instantiating the mailchimp embed. @experimatt are we using these anywhere outside of that embed? I do like the idea of centralizing the imports here,
There was a problem hiding this comment.
Nope, just in the embed. I can move it later, now that we've got separate scss files for each component.
| @import '~@blueprintjs/core/lib/css/blueprint.css'; | ||
| @import '~@blueprintjs/icons/lib/css/blueprint-icons.css'; | ||
| @import '~@blueprintjs/core/lib/css/blueprint.css'; | ||
| @import '~@blueprintjs/icons/lib/css/blueprint-icons.css'; |
There was a problem hiding this comment.
It looks like all the blueprint imports are twice here (same as it was in App.css, dating back to our work session a few weeks ago -- I don't think this was introduced in this PR). We can probably remove the imports on line 3 and 4 at some point.
Alright here we go, this PR has:
node-sass-chokidarThe Create React App README still has this as the preferred method for adding any preprocessors
.gitignorefile to exclude CSS files in source control, we only need them during the buildApp.scssimports everything, similar to the JS files_variables.scssis where colors, fonts, and mixins go_ common.scssis for overall things, likebodyh1-h6hr_header.scssis only for the nav area_sections.scssare styles per each section