-
Notifications
You must be signed in to change notification settings - Fork 67
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
Enforce pseudo-selector ordering by increasing specificity #1667
Conversation
🦋 Changeset detectedLatest commit: 7e5a666 The changes in this PR will be included in the next version bump. This PR includes changesets to release 7 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
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.
Just a review on this following #1666, I think this all makes sense in conjunction with that PR. No gaps I see, but I did not fully follow the AST and code; I can give a more thorough review of required after that lands, but nothing stood out worth that review from me today.
OnceExit(root) { | ||
root.walkRules((rule) => { | ||
rule.selectors = rule.selectors.map((selector) => { | ||
if (selector.includes('._')) { |
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 isn't configured or a constant, right? Given the circular dependencies issues in the past, not worth the effort, just making sure it doesn't already exist!
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 doesn't feel right. We assume Atmoic CSS classname starts with ._
. The assumption might not be correct If we change the classname pattern in the future. We might forget about this if condition here. (TBH, I don't know why we would change classname pattern in the future. Maybe restart classname compression)
I think you won't need to check selector.includes('._')
, If you do enforcePseudoOrder
as part of atomicifyRules
.
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.
- Move
enforcePseudoOrder
into theatomicifyRules
stage, where we no longer need to worry about accidentally processing non-Compiled selectors
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.
oh hmm i suspect there is no need to move enforcePseudoOrder
into atomicifyRules
, as all of the PostCSS plugins should only run on Compiled CSS... haven't confirmed this though
- Confirm that plugins in
packages/css/src/plugins/
never process non-Compiled 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.
Ah, well, yeah something to confirm I'm unsure unfortunately! Would depend on what is picked up in the bundler plugins.
|
||
export const getPseudoSelectorScore = (selector: string): number => { | ||
const index = styleOrder.findIndex((pseudoClass) => selector.trim().endsWith(pseudoClass)); | ||
return index + 1; |
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 wonder if you even need the +1
? I don't care much!
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.
yepp i plan to clean up and de-dupe these util functions once the other PR gets merged in
looks like packages/jest/
will need its own copy of these util functions though - i ran into the same problem as you did, where i can't import @compiled/utils
from inside @compiled/jest
i'm hesitant to do the cleanup prematurely cuz there is overlap in the files modified across the two PRs, and dealing with merge conflicts is not fun (that was my bad for not coordinating this more efficiently)
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
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.
- De-duplicate
packages/utils/src/style-ordering.ts
0840a34
to
5e229cd
Compare
@@ -49,6 +51,7 @@ export const transformCss = ( | |||
classNameCompressionMap: opts.classNameCompressionMap, |
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.
There are quite a lot of references of classNameCompression
in the code base. Since we don't support this feature, we should clean them up.
I am happy to do it after this PR.
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.
lets not clean up classNameCompression
from elsewhere in the codebase yet, as we might use it for future performance projects
5e229cd
to
96264ed
Compare
closing this PR as nobody is working on this at the moment, and changing the specificity is highly risky feel free to re-open if this feature is still needed though! |
Add the
enforcePseudoOrder
option. This ensures that the pseudo-selectors from Compiled are always applied in the order:link
:visited
:focus-within
:focus
:focus-visible
:hover
:active
even when components are imported from multiple packages. This is at the expense of lengthening those selectors considerably (by adding
:not(#\\##\\##\\##\\##\\#)
to the end of those selectors).This is necessary if you use multiple pseudo-selectors on the same component (e.g.
:hover
and:active
), and if you are creating Compiled components that are consumed in other packages or projects.Another approach we could have taken for this was to use
@layer
, but this was determined to be impractical as styles in@layer
have lower specificity than styles outside@layer
, which is risky when mixing Compiled styles with non-Compiled styles. We want Compiled styles to have higher specificity! (This might change in the future depending on whether we care about using Compiled alongside other CSS solutions* or not...)This is turned off by default. To turn on, set
enforcePseudoOrder
totrue
in the Parcel, Webpack, or@compiled/babel-plugin
configuration.Todos for myself
packages/css/src/plugins/__tests__/enforce-pseudo-order.ts
@compiled/jest
helper function to work with this (and potentially theincreaseSpecificity
option too)*other CSS solutions = other CSS-in-JS libraries, and using plain class names (
<div className="hello" />
)