(fix) Bug when extending styles (#218) #220
Conversation
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 5aa0f0b:
|
|
Size Change: 0 B Total Size: 3.56 kB ℹ️ View Unchanged
|
| export let update = (css, sheet, before) => { | ||
| sheet.data.indexOf(css) == -1 && | ||
| (sheet.data = before | ||
| ? sheet.data.replace('/*' + before[0] + '*/', css + '/*' + before[0] + '*/') |
There was a problem hiding this comment.
Alrighty, couldn't find time to look into it properly, but now I did. You did an excellent job in here! 😄 really love it!
Do you think we could actually use the bellow instead of adding a comment? 🤔
| ? sheet.data.replace('/*' + before[0] + '*/', css + '/*' + before[0] + '*/') | |
| ? sheet.data.replace('.' + before[0], css + '.' + before[0]) |
If we do the above, the changes to hash.js are not needed, and that makes the size hit around +10B which is really awesome to fix this bug! 🎉
There was a problem hiding this comment.
Thanks for taking a look at this.
Well in some cases (like media queries) I think there might be some issues with your suggestion.
Like in this case
const RedBase = styled('div')`
@media (max-width: 1245px) {
color: #FF2222;
}
@media (max-width: 800px) {
color: #FF4444;
}
`;In that case if you try to use sheet.data.replace('.' + before[0], css + '.' + before[0]) it would be a problem:
@media (max-width: 12450px) {
/* THE NEW STYLE WOULD BE INSERTED HERE CAUSING A BUG */
.go2087537148{color:#FF2222;}
}
@media (max-width: 800px) {
.go2087537148{color:#FF2222;}
}But adding comments before every style definitely feels hacky...
There's probably a better solution, it might be interesting to look at how styled-components or emotion handle this problem.
There was a problem hiding this comment.
Oh did not thought about media queries. Can you make me understand why that would bug out?
Tried it with the changes in this codesandbox https://codesandbox.io/s/vanilla-forked-wprg1?file=/src/index.js:247-390 and to my untrained eye, looks fine? 🤔
There was a problem hiding this comment.
oh indeed my example did not work
I forked your codesandbox : https://codesandbox.io/s/vanilla-forked-h06f6?file=/src/index.js
You can see here that RedBase is black instead of red, because the class for red is inserted inside the first media-query for the orange style.
There was a problem hiding this comment.
Uh oh! Indeed!
So, that's happening because there are no other styles outside the media queries. Do you see this as an established pattern? To have a component solely based on media queries? 🤔
This works with the above
const Orange = styled(RedBase)`
font-weight: normal;
@media (min-width: 20000px) {
color: orange;
}
@media (min-width: 2px) {
color: orange;
}
`;
| @@ -35,7 +35,7 @@ function styled(tag, forwardRef) { | |||
| // Set a flag if the current components had a previous className | |||
| // similar to goober. This is the append/prepend flag | |||
| // The _empty_ space compresses better than `\s` | |||
| _ctx.o = / *go\d+/g.test(_previousClassName); | |||
| _ctx.o = (_previousClassName || '').match(/ *go\d+/g); | |||
|
@atersolis what do you think about this one? I'm really on the fence because of the size increase 😞 do you think having a note about the known issue for the media query above will help? And merge in the replace method? |
|
I don't know if this pattern is common, but it can happen. |
|
We are having this same issue, even if the bundle increase the library won't have bugs like this! |
|
What's the status of this PR? If this bug isn't fixed then |
@buchansm what's the status of your financial contribution https://opencollective.com/goober? |
A proposal to fix #218
I couldn't find anything better
It adds a comment like /go4214727443/ before every style in the stylesheet so that you can easily insert other styles right before it.
This way the styles are declared in the right order.
Unfortunately it adds around 30 bytes. 1kb sure is small... It's hard to make some changes without significantly increasing the bundle size.
I hope you can find a better fix.