-
Notifications
You must be signed in to change notification settings - Fork 1.2k
[polaris-migrator] Add admin migration from sass media-queries to @custom-media #10865
Conversation
4c61138
to
40ddb37
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.
🙌
Results of this migration can be seen in action in |
AtRule(atRule) { | ||
if (atRule.name === 'media' || atRule.name === 'container') { | ||
atRule.params = atRule.params.replace( | ||
/#\{media-queries\.\$(?<breakpoint>[^}]*)}/g, |
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.
Can we make the namespace (e.g. media-queries.
) a CLI flag? For example, see the tokenize-font migration and associated tests.
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.
Sure... do we know of other codebases that would want to apply the migration and use a different namespace?
This was the reason that pushed me to just call this admin-
and not make it more generic.
I think constructing the regex out of strings is going to get a bit more hairy than this, but definitely makes sense if we think this can be generalized 🤔
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.
The Polaris migrator is usable by Polaris, Web, 1Ps, and 3Ps. If this migration is admin specific, can you add it to web/tooling/stylelint/rules
?
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.
That makes sense... do you have examples of some of those 1Ps I could check?
I think this migration is at the moment a bit specific since it relies on the way previous polaris version were managed which might have taken a different path on different codebases.
I'd like to see if this can be applied to other codebases and how to generalize it if that's the case before making a final call on this.
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.
do you have examples of some of those 1Ps I could check?
I'd like to see if this can be applied to other codebases and how to generalize it
I think an optional namespace should be sufficient
I think constructing the regex out of strings is going to get a bit more hairy than this
Let me know if you need assistance 👍
Hi! We noticed there hasn’t been activity on this PR in a while. After 30 days, it will close automatically. If it’s still relevant, or you have updates, comment and let us know. And don’t worry, you can always re-open later if needed. |
Similar to #10749 Going to close here since we ran this once for web and haven't needed it since. We can always come back and bring this back if needed, but probably better to keep the migrator as lean as we can to avoid bloat. |
WHY are these changes introduced?
This is a companion PR to Add support for @custom-media for using breakpoints in CSS.
It adds a migration to replace existing usages of polaris scss media queries in admin to the new
@custom-media
approach.WHAT is this pull request doing?
Basically, it turns
into
Additionally, it removes the jscodeshift preset as per @aaronccasanova and @sam-b-rose comments here
How to 🎩
I think tests should be enough, but here are some things to take into account:
@media
and@container
since that's the only usage we currently have and not everywhere media queries are allowed@use
rule to simplify the migration. This can be don by running theremote-unused-at-rule
migration lateradmin-
for the rule since it mostly applies to admin codebase, but happy to put under thescss-
orsyles-
umbrellas if we think there's value there🎩 checklist
README.md
with documentation changes