Improved media query sorting #1438
Conversation
abhakat
requested review from
ezzak,
mellyeliu,
necolas,
nmn and
vincentriemer
as code owners
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
bot
added
the
CLA Signed
workflow: benchmarks/perfComparison of performance test results, measured in operations per second. Larger is better.
|
workflow: benchmarks/sizeComparison of minified (terser) and compressed (brotli) size results, measured in bytes. Smaller is better.
|
abhakat
force-pushed
the
improved-media-query-sorting
branch
from
bd9b294 to
0907967
Compare
mellyeliu
left a comment
mellyeliu
left a comment
There was a problem hiding this comment.
Couple changes! Let's also add this behind a config within processStylexRules so we can roll it out easily - there are breakpoints that will shift internally.
| 0: { | ||
| className: "xrvj5dj x1rkzygb x7k18q3 x17lh93j xmr4b4k xesbpuc x15nfgh4", | ||
| "data-style-src": "../../../../../../../../../html/js/FooBar.react.js:11; ../../../../../../../../../html/js/FooBar.react.js:16" | ||
| "data-style-src": "../../../../../../../html/js/FooBar.react.js:11; ../../../../../../../html/js/FooBar.react.js:16" |
There was a problem hiding this comment.
These are irrelevant, you might need to do a clean yarn install
| return classname1.localeCompare(classname2); | ||
| } else { | ||
| if (rule1.startsWith('@') && !rule2.startsWith('@')) { | ||
| if (rule1.startsWith('@') && rule2.startsWith('@')) { |
There was a problem hiding this comment.
We don't want to affect the sorting of other at-rules like @keyframes or similar. Let's limit this to media queries first (@media)
…imit breakpoint sorting to @media rules only.
…verify that the sorting order is correct
mellyeliu
left a comment
mellyeliu
left a comment
There was a problem hiding this comment.
Please add some more tests with different min-width, max-width, with px and other units so we can test the logic
| :root, .xbiwvf9{--small-x19twipt:2px;--medium-xypjos2:4px;--large-x1ec7iuc:8px;} | ||
| @media (prefers-color-scheme: dark){:root, .xsg933n{--colorTokens-xkxfyv:lightblue;}} | ||
| @media (min-width: 600px){:root, .xsg933n{--marginTokens-x8nt2k2:20px;}} | ||
| @media (prefers-color-scheme: dark){:root, .xsg933n{--colorTokens-xkxfyv:lightblue;}} |
There was a problem hiding this comment.
ideally we don't have these change relative to other queries so we introduce less breaking change
| <div id="test" className="color-x1e2nbdu" data-style-src="npm-package:../../../../../../../../../js/node_modules/npm-package/dist/components/Foo.react.js:4">Hello World</div> | ||
| <div className="test" className="color-x1e2nbdu" data-style-src="npm-package:../../../../../../../../../js/node_modules/npm-package/dist/components/Foo.react.js:4" id="test">Hello World</div> | ||
| <div id="test" className="color-x1e2nbdu" data-style-src="npm-package:../../../../../../../../../js/node_modules/npm-package/dist/components/Foo.react.js:4" className="test">Hello World</div> | ||
| <div id="test" className="color-x1e2nbdu" data-style-src="npm-package:../../../../../../../js/node_modules/npm-package/dist/components/Foo.react.js:4">Hello World</div> |
There was a problem hiding this comment.
rebase and yarn install and rerun the tests, this should be fixed.
3 participants
What changed / motivation ?
Linked PR/Issues
Better media query sorting #1407
Additional Context
Ran and updated tests.
Pre-flight checklist
Contribution Guidelines