-
Notifications
You must be signed in to change notification settings - Fork 72
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
[BD-46] feat: Paragon theme CSS is built and published independently of consuming applications #2102
[BD-46] feat: Paragon theme CSS is built and published independently of consuming applications #2102
Conversation
Thanks for the pull request, @PKulkoRaccoonGang! When this pull request is ready, tag your edX technical lead. |
c39ee86
to
1eb1263
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## alpha #2102 +/- ##
=======================================
Coverage 90.76% 90.76%
=======================================
Files 213 213
Lines 3499 3499
Branches 843 843
=======================================
Hits 3176 3176
Misses 315 315
Partials 8 8 ☔ View full report in Codecov by Sentry. |
1eb1263
to
3740366
Compare
3740366
to
ba8dd0a
Compare
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.
@PKulkoRaccoonGang left initial feedback, I think we can also improve build-tokens.js
file more, I'll take a look a little later
scss/core/_utilities.scss
Outdated
@@ -15,7 +15,7 @@ | |||
@import "~bootstrap/scss/utilities/stretched-link"; | |||
@import "~bootstrap/scss/utilities/visibility"; | |||
@import "./bootstrap-override/utilities"; | |||
@import "css/utility-classes"; | |||
@import "css/light/utility-classes"; |
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.
[curious/clarification] Given that _utilities.scss
is imported into core.scss
which is compiled to core.css
, should we be including anything related to a specific theme variant (e.g., light) in this file? Or should these utilities classes be imported into the light.css
file instead?
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, makes sense, we moved it to to the light.css
file
build-scss.js
Outdated
const compileThemeStyleSheets = (themeVariant) => { | ||
fs.writeFileSync( | ||
`./dist/${themeVariant}.css`, | ||
compileStyleSheet(`./scss/core/css/${themeVariant}/variables.css`).css |
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.
[consideration] We may want to generate these CSS files based on the tokens outside of the scss
directory (i.e., its beginning to feel a bit odd to generate CSS files in a folder that intends to be for SCSS files haha).
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.
True, I had similar thoughts haha. I've refactored our styles structure by creating new root styles
directory with separate css
and scss
subdirectories there, this is a breaking change for the consumers, but so is almost every change in alpha
branch, so I think that's ok. (it just didn't feel right to create a separate css
directory without grouping it with scss
, I think introducing styles
directory is more intuitive)
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 think introducing a styles
directory makes sense! Would it contain both a scss
and css
directory?
build-scss.js
Outdated
@@ -12,12 +11,35 @@ var tildaImporter = function(url, prev, done) { | |||
return { file: url }; | |||
}; | |||
|
|||
const compileStyleSheet = (path, output = 'expanded') => { | |||
return sass.renderSync({ |
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.
[curious] Does compileStyleSheet
also generate map files when its output file is compressed? Say an MFE has loaded the core.min.css
file, but wants to debug it. Given its minified, it won't be all that useful for debugging unless we ensure to also ship with an associated *.css.map
file. According to the Dart SASS docs, it generates map files by default.
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.
It didn't generate map files at first, but now it does 🙂 Good suggestion, thanks!
build-scss.js
Outdated
@@ -12,12 +11,35 @@ var tildaImporter = function(url, prev, done) { | |||
return { file: url }; | |||
}; | |||
|
|||
const compileStyleSheet = (path, output = 'expanded') => { | |||
return sass.renderSync({ |
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.
Looks like sass.renderSync
may be deprecated. This might be a good time to try to move away from renderSync
in favor of what Dart SASS recommends for its replacement (i.e., compile
- docs).
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.
Let's at least do discovery to see if we can replace; if not, defer for now.
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.
Turns out we can pretty easily, I don't know why I thought it wasn't possible 🤦. Updated, thanks!
scss/core/core.scss
Outdated
@import "css/custom-media-breakpoints"; | ||
@import "css/core/variables"; | ||
@import "css/core/custom-media-breakpoints"; | ||
@import "css/light/variables"; |
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.
[curious/clarification] Similar question to the css/light/utility-classes
import above, should core.scss
be including the css/light/variables
or only the css/core/variables
given the light variables are also defined in light.css
itself.
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.
We've updated these import too, good catch!
scss/core/core.scss
Outdated
@import "css/variables"; | ||
@import "css/custom-media-breakpoints"; | ||
@import "css/core/variables"; | ||
@import "css/core/custom-media-breakpoints"; |
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.
[curious] Have we been able to verify the new @custom-media
CSS media queries are usable solely based on the output core.css
and light.css
files?
We did add the postcss-custom-media
PostCSS plugin to @edx/frontend-build
to ensure the Webpack config for MFEs can transform @custom-media
definitions into native CSS. While this is useful for MFEs (e.g., they can define/use their own @custom-media
queries), I realized it may not be as relevant if MFEs are injecting an already compiled CSS file into the app. This is because the already-compiled CSS file would not be going through Webpack; it's injected at runtime, not build time.
Given this, I wonder if we may need to look into whether we need to either:
- Only compile SCSS -> CSS with PostCSS (and any necessary plugins like
postcss-custom-media
) instead of Dart SASS. - Compile SCSS -> CSS with Dart SASS and then take the output from Dart SASS and run it through PostCSS.
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.
We've chosen to go with second approach for now to first compile SCSS with Dart SASS and then process resulting CSS with PostCSS.
We could probably install a bunch of plugins to allow PostCSS compile SCSS and drop Dart SASS this way, but I think PostCSS is intended to work with CSS files only and mixing SCSS here would be messy (at least for me haha). I like the separation of concerns we introduce be leavng Dart SASS, i.e. Dart SASS is responsible only for SCSS compilation and PostCSS is responsible for transforming resulting CSS files.
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 completely agree here :)
ced516b
to
e986834
Compare
8cd4d48
to
2c2d961
Compare
2c2d961
to
29b86a7
Compare
@PKulkoRaccoonGang 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
🎉 This PR is included in version 21.0.0-alpha.18 🎉 The release is available on: Your semantic-release bot 📦🚀 |
…of consuming applications (openedx#2102)
…of consuming applications (openedx#2102)
🎉 This PR is included in version 22.0.0-alpha.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 23.0.0-alpha.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 23.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Description
Paragon CLI
Core CSS
Light CSS
NPM
Issue: #2014
Merge Checklist
example
app?wittjeff
andadamstankiewicz
as reviewers on this PR.Post-merge Checklist