-
Notifications
You must be signed in to change notification settings - Fork 483
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
Generate apps/src/color.js from shared/css/color.scss #7367
Conversation
Enforce non-terminal mode for readline.createInterface() See jashkenas/coffeescript#1078 (comment)
colorScssPath + ':' + currentLine, | ||
line, | ||
' ^' | ||
].join('\n')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did my best here to mimic standard node error messages, which makes this stand out nicely when something goes wrong. Here's an example failure running npm run build
when there's an unresolvable reference in color.scss:
Running "exec:buildColorJs" (exec) task
>> /home/brad/Projects/code-dot-org/apps/script/build-color-js.js:52
>> throw new Error([
>> ^
>> Error: Unable to resolve color broken_color
>> /home/brad/Projects/code-dot-org/shared/css/color.scss:10
>> $broken_color: $teal;
>> ^
>> at Interface.<anonymous> (/home/brad/Projects/code-dot-org/apps/script/build-color-js.js:52:11)
>> at Interface.emit (events.js:107:17)
>> at Interface._onLine (readline.js:214:10)
>> at Interface.<anonymous> (readline.js:344:12)
>> at Array.forEach (native)
>> at Interface._normalWrite (readline.js:343:11)
>> at ReadStream.ondata (readline.js:91:10)
>> at ReadStream.emit (events.js:107:17)
>> at readableAddChunk (_stream_readable.js:163:16)
>> at ReadStream.Readable.push (_stream_readable.js:126:10)
>> Exited with code: 1.
Warning: Task "exec:buildColorJs" failed. Use --force to continue.
Note, this sort of resolution error is equally problematic for scss (which doesn't hoist variables) so that's not some new rule we're imposing on that file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth a comment in colors.scss? Any way to enforce it (I think no?)
Note: We have some color info in sharedJsxStyles.js that probably becomes unnecessary with this change. |
var colorJsPath = path.resolve('./src/color.js'); | ||
|
||
// Regular expression to capture a color definition from SCSS | ||
var colorRe = /\$(\w+)\s*:\s*([^;]+);/; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This regex captures any scss variable definition, so we're sort of creating a new constraint on color.scss that says we should keep it relatively simple and only define colors there. I think that was always implied, but this script formalizes it somewhat.
This is pretty sweet. I semi-intended to do something like this at one point, but decided it would take me more time than I wanted. Thanks for doing it! :) |
Review comments addressed; PTAL. |
charcoal: '#5b6770', | ||
green: '#b9bf15', | ||
white: '#fff', | ||
orange: '#ffa400', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make more sense to get rid of this file completely, and add remaining colors to the scss file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, maybe; I went conservative not moving anything out of apps, but since there's nothing but colors in this file right now maybe that's the right thing to do. I was going to say other common styles / style bundles might eventually live here, but personally I prefer doing that case-by-case anyway (e.g. ToggleButtonStyles.js). I'll tear this out before I merge.
Generate apps/src/color.js from shared/css/color.scss
26954f8 Merge pull request #7369 from code-dot-org/icon-color-picker (Josh Lory) 53cfb6e add column in cdo-languages for which languages are supported on hoc.com (Continuous Integration) 7ca9bc8 Merge pull request #7371 from code-dot-org/use-color-js (Brad Buchanan) 3761f86 fix path to unbreak build (Brent Van Minnen) f4526fa Merge branch 'staging' into icon-color-picker (Josh Lory) ba63a4d Code review feedback (Josh Lory) 42b176a Merge pull request #7367 from code-dot-org/generate-color-js (Brad Buchanan)
Given:
Then:
I decided the best way to do this would be a build step that generates cdo/apps/src/color.js by parsing cdo/shared/css/color.scss.
color.js is not used yet - I've just extracted this from my GameLab Animation Tab work, where I will be using it.
Here's the generated file in its entirety; it's
.gitignore
d and not checked in, because it's entirely derived from color.scss. This file is regenerated before every apps build or test run (though not by the apps watcher, since the build step crosses apps/shared boundaries).