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.

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

Transforms only work if applied after classes like rotate, translate #20

Closed
owaiswiz opened this issue Mar 25, 2020 · 12 comments
Closed
Labels
bug Something isn't working

Comments

@owaiswiz
Copy link

owaiswiz commented Mar 25, 2020

tw.`rotate-90 transform` works as expected
however, tw.`transform rotate-90` doesn't work.

Seems anything set by transform isn't overridden later, so since transform already sets --transform-rotate: 0, rotate-90 is not applied. Same goes for all scale,skew, translate classes.
Also using rotate-90! does nothing.
It all only works if the order is reversed. TailwindCSS makes all rules set by these secondary utils (rotate, skew, etc) as !important so they override.

Thank you for the amazing library btw 😄

@n00b2pr0
Copy link

I'm noticing a similar behavior with classes compiling in reverse order.

Example:

// This
css={tw`m-0 mr-4`}

// Compiles to
.component {
  margin-right: 1rem;
  margin: 0;
}

// Expected
.component {
  margin: 0;
  margin-right: 1rem;
}

@ben-rogerson
Copy link
Owner

Damn, those are some fairly huge bugs. I'll get them fixed up pronto. Big thanks for pointing these out!

@ben-rogerson
Copy link
Owner

I've put together a test codesandbox on these issues and it seems to be ordering the margin classes correctly and the transforms work correctly.
To demonstrate the issue, it would be great if you could fork that repo in codesandbox and trigger the errors mentioned?

TailwindCSS makes all rules set by these secondary utils (rotate, skew, etc) as !important so they override.

By looking through the css tailwind builds, I can't see any !importants being added to the transform classes - do you have a source I can see on this?

@owaiswiz
Copy link
Author

owaiswiz commented Mar 26, 2020

I've put together a test codesandbox on these issues and it seems to be ordering the margin classes correctly and the transforms work correctly.
To demonstrate the issue, it would be great if you could fork that repo in codesandbox and trigger the errors mentioned?

I saw the test sandbox. Downloaded it as zip and ran it manually on my local machine, the same issue happens (with a fresh npm install). Tried it on a different browser too, same thing. Have you tried running it locally ? If yes and it works, then there's probably something wrong on my end.

By looking through the css tailwind builds, I can't see any !importants being added to the transform classes - do you have a source I can see on this?

Sorry about that, while creating this issue I was looking at the tailwind docs for transform utilites, inspected their html and they had it set as important so I naively assumed that (seems they have !important on each of their utilites)
screenshot

@ben-rogerson
Copy link
Owner

ben-rogerson commented Mar 27, 2020

So strange, it's working correctly locally:

image

I also tested on these node versions just to make sure:

  • v8.17.0
  • v9.11.2
  • v10.18.1
  • v11.15.0

Edit: I realise I didn't have a proper 'clean slate' between testing versions here.

@n00b2pr0
Copy link

Thanks for looking into this @ben-rogerson!

The project I discovered this issue uses Emotion and Gatsby. I forked the Gatsby + Tailwind + Emotion starter and was able to replicate the issue: https://codesandbox.io/s/twin-ordering-emotion-test-7b472

image

I've dug in a little and not closer to why, but glad I could produce an example outside my project.

@ben-rogerson
Copy link
Owner

ben-rogerson commented Mar 28, 2020

I've made a minimal bug reproduction here.

I've noticed different behaviour between node versions when installing the reproduction locally.

Ordering issue present

1. Download zip from codesandbox + unzip
2. nvm use 10 (node v10.18.1 + npm v6.13.4 - same as codesandbox uses)
3. yarn && yarn start

image

Ordering working correctly

1. Download zip from codesandbox + unzip
2. nvm use 11 (node v11.15.0 + npm v6.7.0)
3. yarn && yarn start 

image

Not sure what's triggering the issue within node v10.18.1 yet.

@owaiswiz
Copy link
Author

owaiswiz commented Mar 28, 2020

Not sure what's triggering the issue within node v10.18.1 yet.

I think I figured it out. It seems node (V8 really) switched to a stable sorting algorithm with v11 ( #22754, Getting things sorted in V8 )

The problem stems from here:

const orderByScreens = (classNames, screens) =>
classNames.sort((a, b) => {
const A = a.includes(':') ? a.split(':')[0] : a
const B = b.includes(':') ? b.split(':')[0] : b
return screens.indexOf(A) < screens.indexOf(B) ? -1 : 1
})

For example, if I test this on node v10.18.1, this is what I get:
nodess

And this is what we get on newer node versions (which uses the updated v8) (using my browser here, dont have node v11 installed right now)
correctss

A possible fix would be to use this sorting function - timsort.

@ben-rogerson ben-rogerson added the bug Something isn't working label Mar 29, 2020
@ben-rogerson
Copy link
Owner

I've added timsort and it's working perfectly when installed locally in node 10.
But it's still not working in codesandbox? 🤷

@owaiswiz
Copy link
Author

I've added timsort and it's working perfectly when installed locally in node 10.
But it's still not working in codesandbox? 🤷
working as expected for me
ss

@ben-rogerson
Copy link
Owner

ben-rogerson commented Mar 30, 2020

This issue is solved! 🎉

We've reached closing time.

@n00b2pr0
Copy link

Great! 1.0.0-alpha.8 works for me. 🎉

Thanks @ben-rogerson and @owaiswiz! 👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants