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

CSS ordering/specificity is incorrect #16

Merged
merged 17 commits into from
May 24, 2019
Merged

CSS ordering/specificity is incorrect #16

merged 17 commits into from
May 24, 2019

Conversation

ericclemmons
Copy link
Collaborator

@ericclemmons ericclemmons commented May 24, 2019

When replacing tailwind.css with benefit.css for https://github.com/tailwindcss/playground (https://epic-edison-761ca3.netlify.com/), things like md:pl-4 pl-0 don't behave as expected.

There are a couple things to address:

  • css.js shouldn't sort classes at all when outputting them.
  • Classes should be generated such that specific rules are added after generic rules. (e.g. .bg-white, then .sm:bg-blue, then .sm:bg-black:hover)
  • styleWith should respect the ordering of classes from the above efforts, vs. emotion enforcing left-to-right prioritization.

@cdonohue cdonohue self-assigned this May 23, 2019
@ericclemmons
Copy link
Collaborator Author

Confirmed fixing this corrects the CSS issue: classes are compiled in the order they're listed in Object.keys(utilities) (which is deterministic):

Screen Shot 2019-05-23 at 7 42 25 PM

@ericclemmons
Copy link
Collaborator Author

Looking into the behavior when using styleWith now...

@ericclemmons
Copy link
Collaborator Author

Enforcing order with styleWith ea8a0f4

Notice how we see the reset class, the md:pl-16 class, and the pl-0 class.

There's a potential that, if these rules are called individually, then we can't guarantee that their order matches that of `utilities.

Enforcing order with cx 59a643b

This builds on ea8a0f4 in that, instead of concatenating strings, we pass it to cx to ensure that the rules are compiled in an order to ensure it's deterministic:

Screen Shot 2019-05-23 at 8 43 24 PM

Screen Shot 2019-05-23 at 8 56 35 PM

I feel that this version is the safest approach: changing order of utilities will be all that's necessary to enforce order.

@ericclemmons
Copy link
Collaborator Author

The most controversial part is b657408, where normalizeClass is removed from each utility.

We've discussed this before, and it seems with more & more examples that normalization per-class (or per-usage) is overkill, as it's attempting to neuter a cascade that doesn't exist (or that we would want via base.css).

My recommendation would be for "sandboxing" to be done via a flag (like important), or a isolateWith(...) version.

@ericclemmons ericclemmons added the bug Something isn't working label May 24, 2019
@cdonohue cdonohue assigned ericclemmons and unassigned cdonohue May 24, 2019
@ericclemmons
Copy link
Collaborator Author

Sweet!

  • styleWith keeps original behavior.
  • cx uses emotion's cx utility to enforce order & flattens classes down.
  • normalization has been removed by default so it can be added in React land.

Copy link
Owner

@cdonohue cdonohue left a comment

Choose a reason for hiding this comment

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

Love this! @ericclemmons just update the jest snapshots then we can merge this.

@ericclemmons ericclemmons mentioned this pull request May 24, 2019
@ericclemmons
Copy link
Collaborator Author

I'm an idiot. Tests were passing until I added cx.

Didn't realize you had CodeShip!

@ericclemmons ericclemmons mentioned this pull request May 24, 2019
@cdonohue cdonohue merged commit 8aba970 into master May 24, 2019
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

Successfully merging this pull request may close these issues.

2 participants