-
Notifications
You must be signed in to change notification settings - Fork 66
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: added CSS custom media query transformer #2068
[BD-46] feat: added CSS custom media query transformer #2068
Conversation
Thanks for the pull request, @PKulkoRaccoonGang! When this pull request is ready, tag your edX technical lead. |
Codecov ReportBase: 90.77% // Head: 90.77% // No change to project coverage 👍
Additional details and impacted files@@ Coverage Diff @@
## alpha #2068 +/- ##
=======================================
Coverage 90.77% 90.77%
=======================================
Files 213 213
Lines 3501 3501
Branches 843 843
=======================================
Hits 3178 3178
Misses 315 315
Partials 8 8 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
6254fba
to
9184d80
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.
It's great seeing how straightforward it is to implement stuff using tokens! Left a comment with a question on here but overall this is just really exciting stuff!
scss/core/core.scss
Outdated
@@ -1,4 +1,5 @@ | |||
@import "css/variables"; | |||
@import "css/custom-media"; |
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.
is the plan to add more than just breakpoints to this file? it seems we have a couple options going forward:
- add all media query related stuff to
custom-media
(media queries cover a range of things, not sure if following that is how we want to group our files or not) - add all breakpoints (
max-width
queries) tocustom-breakpoints
(or a similarly named file with the same limited scope), add other media queries (orientation
etc.) to other.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.
These are good ideas! I think we have opportunities for flexible usage thanks to Post CSS and plugins "custom-media".
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'd lean a bit more towards creating separate formats for the different types of media queries, starting with the custom-breakpoints
in this PR (though, maybe custom-media-breakpoints
?).
In the future, we could expanding the set of custom media queries around things like device width, etc. (i.e., media queries that depend on CSS variable(s) generated from a design token).
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.
Yes, I think custom-media-breakpoints
is a more obvious naming variant 👍
fadbe74
to
b0eb33e
Compare
@@ -280,9 +280,9 @@ jobs: | |||
uses: hmarr/auto-approve-action@v2 | |||
with: | |||
pull-request-number: ${{ steps.cpr.outputs.pull-request-number }} | |||
github-token: ${{ secrets.requirements_bot_github_token }} | |||
github-token: ${{ secrets.EDX_NETLIFY_PAT }} |
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] Are these changes remnants of dealing with issues in the Git history? Let's keep this using secrets.requirements_bot_github_token
moving forward as that's what's on master
. The EDX_NETLIFY_PAT secret is my hack before I knew requirements_bot_github_token
existed :)
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.
Roll-backed this commit in this pull request
.github/workflows/commitlint.yml
Outdated
@@ -2,7 +2,8 @@ | |||
|
|||
name: Lint Commit Messages | |||
|
|||
on: pull_request | |||
on: |
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.
Similar comment as above for this change.
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.
Roll-backed this commit in this pull request
tokens/style-dictionary.js
Outdated
@@ -154,4 +154,24 @@ StyleDictionary.registerFormat({ | |||
}, | |||
}); | |||
|
|||
/** | |||
* Formatter to generate CSS custom media queries. |
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.
nit: maybe add "custom media queries for responsive 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.
Exactly, thanks
scss/core/core.scss
Outdated
@@ -1,4 +1,5 @@ | |||
@import "css/variables"; | |||
@import "css/custom-media"; |
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'd lean a bit more towards creating separate formats for the different types of media queries, starting with the custom-breakpoints
in this PR (though, maybe custom-media-breakpoints
?).
In the future, we could expanding the set of custom media queries around things like device width, etc. (i.e., media queries that depend on CSS variable(s) generated from a design token).
tokens/style-dictionary.js
Outdated
* 'breakpoints' subcategory, and generates a CSS custom media queries. | ||
*/ | ||
StyleDictionary.registerFormat({ | ||
name: 'css/custom-media', |
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.
As suggested in the previous comment, let's plan on having distinct transforms for different types of media queries. In this case, we're working with responsive breakpoints, so per the suggestion above, perhaps css/custom-media-breakpoints
here?
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.
Done, thanks
StyleDictionary.registerFormat({ | ||
name: 'css/custom-media', | ||
formatter({ dictionary, file }) { | ||
const { size: { breakpoint } } = dictionary.properties; |
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 is neat to see that this wasn't that large of a lift given what we already had! 🚀
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.
Yes, it's very simple thanks to the Style Dictionary setups 💯
b0eb33e
to
af5afdc
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. |
Also merged/released the associated |
🎉 This PR is included in version 21.0.0-alpha.14 🎉 The release is available on: Your semantic-release bot 📦🚀 |
* feat: added CSS custom media query transformer * refactor: refactoring after review * refactor: refactoring after review
* feat: added CSS custom media query transformer * refactor: refactoring after review * refactor: refactoring after review
🎉 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 📦🚀 |
* feat: added CSS custom media query transformer * refactor: refactoring after review * refactor: refactoring after review
* feat: added CSS custom media query transformer * refactor: refactoring after review * refactor: refactoring after review
* feat: added CSS custom media query transformer * refactor: refactoring after review * refactor: refactoring after review
* feat: added CSS custom media query transformer * refactor: refactoring after review * refactor: refactoring after review
Description
@custom-media
from design tokens with style-dictionary.postcss-custom-media
to @edx/frontend-build's default Webpack configs for all MFEs.Issue: #2015
Relative link: Frontend build
Merge Checklist
example
app?wittjeff
andadamstankiewicz
as reviewers on this PR.Post-merge Checklist