feat(#119): add grid prefix support

SukkaW commented 2 years ago

When finished, this should close https://github.com/thysultan/stylis/issues/119 once and for all.

Note: ~~It is still WIP~~ Although the feature is implemented, it hasn't been optimized AT ALL, so suggestions, optimizations, discussions are welcome!

SukkaW commented 2 years ago

TODO:

[x] display: grid / inline-grid
[x] align-self / justify-self: strech / end / center
[x] grid-template-columns
[x] grid-template-rows
[x] grid-template-columns
[x] grid-row-start: numerical: see #287
[x] grid-row-end: numerical: see #287
[x] grid-row-start: non-numerical: see #287
[x] grid-row-end: non-numerical: see #287
[x] grid-column: numerical
[x] grid-row: numerical
[x] grid-column: expand shorthand syntax
[x] grid-row: expand shorthand syntax

SukkaW commented 2 years ago

@thysultan @Andarist

The PR is ready for review. And here is a few things:

Prefixing grid-row-start and grid-row-end requires the knowledge of both declarations, so it is impossible for stylis stateless per declaration processor to prefix them.
I just write the code to make it pass the test cases. It is highly un-optimized, introducing tons of performance overhead and unnecessary byte to the final dist build. Feel free to add your ideas and suggestions!

coveralls commented 2 years ago

Pull Request Test Coverage Report for Build d74963977ee5661da6a526faa4b7120ed49de8e8-PR-285

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

7 of 7 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.04%) to 98.966%

Totals
Change from base Build 66582efbbbf02895797f1109cf736de5421ac9d2:	0.04%
Covered Lines:	267
Relevant Lines:	269

💛 - Coveralls

thysultan commented 2 years ago

Prefixing grid-row-start and grid-row-end requires the knowledge of both declarations

Elaborate?

thysultan commented 2 years ago

so it is impossible for stylis stateless per declaration processor to prefix them

The prefixer middleware, which has access to the complete state of the css token graph does not have this limitation, the prefix function in Prefixer.js is called by this middleware: https://github.com/thysultan/stylis/blob/9e61e05a04060ad397c951877dee311297825568/src/Middleware.js#L42

SukkaW commented 2 years ago

Elaborate?

If grid-row-start and grid-row-end should be prefixed, here is how:

If only grid-row-start exists:
- Prefix with -ms-grid-row. So grid-row-start:3; becomes -ms-grid-row:3;-ms-grid-row:3.
If only grid-row-end exists:
- Prefix with -ms-grid-row-span. So grid-row-end:3; becomes -ms-grid-row-span:3;grid-row-end:3;, and grid-row-end:span 3; becomes -ms-grid-row-span:3;grid-row-end:span 3;
If both of them exists:
- Prefix grid-row-start with -ms-grid-row
- Add -ms-grid-row-span with a value of grid-row-end - grid-row-start
- So grid-row-start:3;grid-row-end:5; becomes -ms-grid-row: 3;grid-row-start:3;-ms-grid-row-span:2;grid-row-end:5;

The same rule applies to column.

I mainly port the features from @robinweser's inline-style-prefixer (He recently switch Fela's prefixer from his own inline-style-prefixer to stylis. Porting the feature helps to keep consistent behavior).

Andarist commented 2 years ago

One concern that I have is that it might potentially increase the output quite a bit for existing users. I wonder if this could be split into 2 plugins somehow? Or should I potentially get ready for copying over the older version of the prefixed into Emotion's codebase? 🤔

robinweser commented 2 years ago

Chiming in since I was mentioned in this thread: Maybe it'd be worth exploring a way to build your own prefixer from browser data or something? Over at inline-style-prefixer, we export a generateData and createPrefixer function which one can use to create your very own one given a list of browser versions. See https://inline-style-prefixer.js.org/docs/guides/CustomPrefixer.html

SukkaW commented 2 years ago

One concern that I have is that it might potentially increase the output quite a bit for existing users. I wonder if this could be split into 2 plugins somehow? Or should I potentially get ready for copying over the older version of the prefixed into Emotion's codebase? 🤔

That's why I said my initial changes are highly unoptimized. And I believe it is possible to reduce the impact down to below 2 KiB or even below 1 KiB, which should then be acceptable.

And currently, the after-gzipped size is only increased by about 200 bytes. That doesn't sound so bad, right?

Andarist commented 2 years ago

And currently, the after-gzipped size is only increased by about 200 bytes. That doesn't sound so bad, right?

I'm not that concerned about the increase of the Stylis' size - but rather about the increased size of the prefixed output. My concern is about the generated CSS.

SukkaW commented 2 years ago

I'm not that concerned about the increase of the Stylis' size - but rather about the increased size of the prefixed output. My concern is about the generated CSS.

Only those grid-related declarations will contribute to the increasing final CSS output. So it really depends on how much grid do the developers use in their project currently: if they use none, then the prefixed output will not be affected at all.

So I've done some naive tests here:

Go to some websites
Grab a dump of their entire CSS
Put the CSS into PostCSS's autoprefixer, with the browserlist set to since 2022, IE 10 to make sure only -ms- prefixes get applied.
Compare the size change.

I've checked dev.to (their source code can be found here), a website whose layout is heavily based on the CSS grid. And the CSS size before and after autoprefixer is 444KiB vs 449KiB.

I've also run the test on https://emotion.sh 's doc page by extracting all inlined <style> tags. The size before and after is 34KiB vs 35.2KiB.

thysultan commented 2 years ago

@robinweser Kind of wish browserlist in bundlers was like eslint in so much as it would support inline comments to pieces of atomic pieces of code i.e

// ie11
if (a) b(c)

Denoting that the code only exists to make something work on ie11, then this data would make it to the bundler and strip it away from supported browser targets that are not ie11.

SukkaW commented 2 years ago

The prefixer middleware, which has access to the complete state of the css token graph does not have this limitation, the prefix function in Prefixer.js is called by this middleware:

@thysultan Done in #287

robinweser commented 2 years ago

@robinweser Kind of wish browserlist in bundlers was like eslint in so much as it would support inline comments to pieces of atomic pieces of code i.e
// ie11
if (a) b(c)
Denoting that the code only exists to make something work on ie11, then this data would make it to the bundler and strip it away from supported browser targets that are not ie11.

Omg, that would be a dream come true!

Especially for something like prefixers where the amount of code and required prefixes differs so much depending on the browser range.

SukkaW commented 2 years ago

@thysultan Thanks for your review! I will remove whitespace (and possibly other style-related) diff first.

thysultan commented 2 years ago

@SukkaW Thanks for the work you've done on this.

thysultan / stylis