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

Update tailwindcss @apply suggestion #5406

Merged
merged 8 commits into from
Nov 21, 2022
Merged

Update tailwindcss @apply suggestion #5406

merged 8 commits into from
Nov 21, 2022

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Nov 15, 2022

Changes

There has been a couple issues about tailwindcss recently:

This PR clarifies that inline classes should be preferred instead of @apply as it does not work for classes like dark:text-pink and duplicates the global and Astro scoped style output.

Question: Should we mentioned that it's not possible to code-split tailwind CSS output per-page too? All pages can currently only share one stylesheet file.

Testing

N/A

Docs

Updated tailwindcss README

@bluwy bluwy requested a review from a team as a code owner November 15, 2022 08:09
@changeset-bot
Copy link

changeset-bot bot commented Nov 15, 2022

⚠️ No Changeset found

Latest commit: bbf09b8

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-actions github-actions bot added the pkg: integration Related to any renderer integration (scope) label Nov 15, 2022
@matthewp
Copy link
Contributor

Update looks good to me.

Copy link
Member

@delucis delucis left a comment

Choose a reason for hiding this comment

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

Thanks @bluwy! If I understand correctly, this text and the code sample that follow are not strictly speaking related to the config.applyBaseStyles option, correct? We just added these there because we were already talking about @layer?

I wonder if we should move this to the “Troubleshooting” section with a dedicated subheading like “Using @apply with components”?

packages/integrations/tailwind/README.md Outdated Show resolved Hide resolved
@bluwy
Copy link
Member Author

bluwy commented Nov 17, 2022

I wonder if we should move this to the “Troubleshooting” section with a dedicated subheading like “Using @apply with components”?

Make sense to me. I wonder why the previous docs were there too but this definitely sounds like a troubleshooting guide. Will move it there.

@bluwy
Copy link
Member Author

bluwy commented Nov 17, 2022

Updated the docs and moved the two @apply issues under troubleshooting. GitHub's diff isn't showing it right, but I didn't touch the examples section.

@tony-sull
Copy link
Contributor

tony-sull commented Nov 17, 2022

For issue (1), is that a Tailwind issue in general, or does it have something to do with how Tailwind picks up the config file in an astro project?

For (3), I think something like h1 { @apply dark:text-gray-50; } should be compiled directly to the code below, with our scoped class added as usual. Maybe there are certain edge cases where it breaks? Scratch that, just realized the @apply dark: bug is only when using Tailwind's darkMode: "class" config

@media (prefers-color-scheme: dark) {
 h1:where(.astro-GFDZHAND) {
  --tw-text-opacity: 1;
  color:rgb(249 250 251 / var(--tw-text-opacity))
 }
}

@bluwy
Copy link
Member Author

bluwy commented Nov 18, 2022

For issue (1), is that a Tailwind issue in general, or does it have something to do with how Tailwind picks up the config file in an astro project?

It's a tailwind issue in general added in #4353. The tailwind docs also outlined this caveat: https://tailwindcss.com/docs/functions-and-directives#using-apply-with-per-component-css

Yup re no3, it only happens for darkMode: "class"

Copy link
Member

@delucis delucis left a comment

Choose a reason for hiding this comment

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

Thanks @bluwy! I think these are really good and provide clear examples of both the problem and what to do instead. I’ve suggested some wording tweaks but then should be good to go 🙌

packages/integrations/tailwind/README.md Outdated Show resolved Hide resolved
packages/integrations/tailwind/README.md Outdated Show resolved Hide resolved
bluwy and others added 2 commits November 21, 2022 23:03
Co-authored-by: Chris Swithinbank <swithinbank@gmail.com>
Co-authored-by: Chris Swithinbank <swithinbank@gmail.com>
Copy link
Member

@delucis delucis left a comment

Choose a reason for hiding this comment

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

Thanks for working through this with me @bluwy!

@bluwy bluwy merged commit 8f203bf into main Nov 21, 2022
@bluwy bluwy deleted the docs-note-tailwind branch November 21, 2022 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: integration Related to any renderer integration (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants