Sort shorthand across multi-properties #1732
Conversation
Example: this `{-webkit-text-decoration-color:initial;text-decoration-color:initial}` blocks all sorting above/below it, effectively creating a new "sorting bucket" because it always returns `0` as `a` or `b`.
```css
._y44v1tgl{animation:kbayob8 5s ease infinite}
._syaz1qtk{color:var(--ds-link)}
._1jmq18uv{-webkit-text-decoration-color:initial;text-decoration-color:initial}
._zulph461{gap:var(--ds-space-050)}
```
Added a test to replicate and fix the issue.
kylorhall-atlassian
requested review from
JakeLane,
pancaspe87,
liamqma,
dddlr and
zesmith2
as code owners
🦋 Changeset detectedLatest commit: cccb610 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
✅ Deploy Preview for compiled-css-in-js canceled.
|
There was a problem hiding this comment.
I noticed this file gets regenerated quite often, I'm not really sure why—if this is developer error or our deps are so prone to changing…? Making CI protect this in #1733 (nevermind, I gave up)
| // WARNING: This is technically wrong as `.a { … }` is not sorted as we expect; | ||
| // it _should_ be 'abcdef' not 'eabcdf'. | ||
| // | ||
| // We are ok with this because we expect atomicifyRules to run before this plugin, | ||
| // so each class will never have more than one property. | ||
| expect(actual).toMatchInlineSnapshot(` | ||
| ".e { border-top: none; } | ||
| " |
There was a problem hiding this comment.
👀 Guess it wasn't what I thought and this was a bug actually!
| if (node.nodes.length === 1 && nodeIsDeclaration(node.nodes[0])) { | ||
| return node.nodes[0]; | ||
| } | ||
| // Return the first node that is a declaration, if we find one |
There was a problem hiding this comment.
This may not be correct at all, eg. if Compiled ever tries to sort this, it's kind of uncharted territory…global will come second here 🥴
.global {padding-left: 8px;all: unset}
._abcd1234{padding: 8px}There was a problem hiding this comment.
I think we could technically do a deep recursive sort somehow, eg. compare the scores of all children vs. another child, but I'm expecting in real-world Compiled, the above is not a legitimate scenario and instead using the first property's "score" is fine.
There was a problem hiding this comment.
I think if I learnt anything from this, it's that it's safer to ask devs not to mix shorthand and longhand properties at all, than to try to work with it 😅
| ._18s8paju{margin:var(--ds-space-150,9pt) auto 28px auto} | ||
| ._y44v1syn{animation:kbpspdk 5s ease infinite} | ||
| ._y44vbxa2{animation:k1qo9wnt 5s ease infinite} | ||
| ._bfhk29zg, ._irr329zg:hover{background-color:var(--ds-background-selected,#deebff)} |
There was a problem hiding this comment.
I suspect sorting pseudos after non pseudos won't (and can't) work for this line?
(Not sure how we would fix this short of splitting up the declaration...)
There was a problem hiding this comment.
Yeah I'm actually confused what we'd do here…it's unrelated to this, but I guess you would have to split this into two… 😢
I think the impact of this is pretty minor, but not nothing!
There was a problem hiding this comment.
yeahhh i'm ok with tackling stuff like ._bfhk29zg, ._irr329zg:hover{background-color:var(--ds-background-selected,#deebff)} being out of scope, but best to keep in the back of our minds in case we ever figure out why this happens
| ._apju8stv:hover .linkText{-webkit-text-decoration-line:underline;text-decoration-line:underline} | ||
| ._ku3unqa1:hover .linkText{-webkit-text-decoration-style:solid;text-decoration-style:solid} | ||
| ._pwyo12x7{padding-inline-end:var(--ds-space-075,6px)} | ||
| ._1b7p1epz{padding-left:var(--ds-space-1000,5pc)} |
There was a problem hiding this comment.
why does padding-left and padding-right come before background
There was a problem hiding this comment.
It's …:active{background} isn't it?
._j6xt1fef:active{background:var(--_hcgrh3)}
._1owrotz2:active, ._jzfzotz2:focus, ._1mq6otz2:hover{padding:0 var(--ds-space-050,4px)}There was a problem hiding this comment.
ah you're right yeah, this looks fine
| const actual = transform(outdent` | ||
| ._zulph461{gap:3px} | ||
| ._bfhk1ayf{background-color:red} | ||
| ._1jmq18uv{-webkit-text-decoration-color:initial;text-decoration-color:initial} |
There was a problem hiding this comment.
ah is this from autoprefixer? I guess the 1 property per class name assumption is incorrect after all
sorry for introducing the bug 😔
There was a problem hiding this comment.
Nah, 99% sure it was there before because I just return 0'd arrays (and this is an array of properties)
What is this change?
Making sure sorting works across mutli-property classes.
Example: this
{-webkit-text-decoration-color:initial;text-decoration-color:initial}currently blocks all sorting above or below it, effectively creating a new "sorting bucket" because it always returns0asaorb.This doesn't get sorted:
Added a test to show how this works now!
PR checklist
I have...
Not updated the documentation inwebsite/