Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

(fix) Bug when extending styles (#218) #220

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

(fix) Bug when extending styles (#218) #220

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ee8865c
(fix) Bug when extending styles (#218)
atersolis Jan 2, 2021
37a2402
Merge branch 'master' into master
cristianbote Jan 6, 2021
cfba9cb
Merge branch 'master' into master
cristianbote Jan 7, 2021
ced6044
Merge branch 'master' into master
cristianbote Jan 12, 2021
5aa0f0b
Merge branch 'master' into master
cristianbote Jan 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions src/core/hash.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,11 @@ let stringify = (data) => {
* @param {String|Object} compiled
* @param {Object} sheet StyleSheet target
* @param {Object} global Global flag
* @param {Boolean} append Append or not
* @param {Boolean} before To insert style before an other style
* @param {Boolean} keyframes Keyframes mode. The input is the keyframes body that needs to be wrapped.
* @returns {String}
*/
export let hash = (compiled, sheet, global, append, keyframes) => {
export let hash = (compiled, sheet, global, before, keyframes) => {
// Get a string representation of the object or the value that is called 'compiled'
let stringifiedCompiled = typeof compiled == 'object' ? stringify(compiled) : compiled;

Expand All @@ -55,7 +55,8 @@ export let hash = (compiled, sheet, global, append, keyframes) => {
}

// add or update
update(cache[className], sheet, append);
//"/*" + className + "*/" is a marker to insert styles before .className
update('/*' + className + '*/' + cache[className], sheet, before);

// return hash
return className;
Expand Down
7 changes: 5 additions & 2 deletions src/core/update.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@ export let extractCss = (target) => {
* @param {Object} sheet
* @param {Boolean} append
*/
export let update = (css, sheet, append) => {
sheet.data.indexOf(css) == -1 && (sheet.data = append ? css + sheet.data : sheet.data + css);
export let update = (css, sheet, before) => {
sheet.data.indexOf(css) == -1 &&
(sheet.data = before
? sheet.data.replace('/*' + before[0] + '*/', css + '/*' + before[0] + '*/')
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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? 🤔

Suggested change
? 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! 🎉

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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? 🤔

Copy link
Author

@atersolis atersolis Jan 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

@cristianbote cristianbote Jan 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
  }
`;

: sheet.data + css);
};
2 changes: 1 addition & 1 deletion src/styled.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,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);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one adds +0B 😄


_props.className =
// Define the new className
Expand Down