Minify Global Styles#132
Conversation
|
|
||
| if (isGlobal) { | ||
| path.replaceWith(makeStyledJsxTag(id, css.source || css, css.modified)) | ||
| const transformedCss = transform(null, (css.source || css)) |
There was a problem hiding this comment.
nothing wrong with this but you could also move let transfromedCss (L306) to L298
giuseppeg
left a comment
giuseppeg
left a comment
There was a problem hiding this comment.
Looks good so far, you may want to pull master.
If you fix the tests I'll merge.
Update: Sorry, a bit to many commits
no worries I'll squash before merging
|
@nikvm we rewrote the css compiler so when you have time you should rebase your changes. |
|
Still on it, by the way. Sorry for the delay 🌴 |
|
@giuseppeg this should do the trick. |
| export default (() => <div data-jsx={793889750}> | ||
| <p data-jsx={793889750}>test</p> | ||
| <_JSXStyle styleId={3149549172} css={"body { background: red }"} /> | ||
| <_JSXStyle styleId={3149549172} css={" body {background: red }"} /> |
There was a problem hiding this comment.
we should suppress those leading whitespaces
There was a problem hiding this comment.
I guess this is a stylis bug, isn't it?
There was a problem hiding this comment.
I might just trim() the transformed css.
There was a problem hiding this comment.
it should be fixable here https://github.com/zeit/styled-jsx/blob/master/lib/style-transform.js#L120-L128
either try to do blob = blob.trim() before L121 or move L127 to L120
There was a problem hiding this comment.
I think that the space is there because of this https://github.com/thysultan/stylis.js/blob/master/stylis.js#L905
There was a problem hiding this comment.
Thanks for the tip @thysultan. I just pushed the fix. 🐛
| null, | ||
| (css.modified || css) | ||
| ) | ||
| } else if (useSourceMaps) { |
There was a problem hiding this comment.
We should have source maps for global styles too I think.
Maybe you can do something like this:
if (useSourceMaps) {
transformedCss = [
transform(
isGlobal ? null : String(state.jsxId),
// ...
),
// ...
]
} else {
transformedCss = transform(
isGlobal ? null : String(state.jsxId),
// ...
)
}There was a problem hiding this comment.
Didn't even think about that. Will fix.
|
@nikvm awesome, the PR looks good thank you! Did you have the chance to test source maps? |
|
@giuseppeg I added a global style tag to the source maps fixture here and the expected output here. But I didn't check the generated mapping comment. |
|
Would love to merge this! Just need to update the tests. Thanks for all your help so far @nikvm |
|
Hi, this would be great to have in styled-jsx, but it's been not merged for two months. Is there something I could do to help get it going? |
5 participants
An attempt to fix #120.
My first thought was to implement a minifier (or import one). But we already got our transformer!
So why not send the global styles through without assigning an id (and adding a suffix to selectors)?
On further thinking about it, it even seems more correct. As of now global styles don't get transformed at all which means no Browser prefixing etc.
Am I missing something? 🙉
Obviously tests don't pass, yet. Also proposed changes need a bit of refactoring/ commenting if accepted. 💩
Update: Sorry, a bit to many commits...