Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Enable Tailwind utility and component classes #675
Enable Tailwind utility and component classes #675
Changes from 8 commits
865cea7
db8cc6f
bc0cdb6
5210e46
ba65a36
621edf4
5b06b47
86c29ae
bc1f3aa
b358d26
dac0e7e
daded58
8410af4
a9ec8ec
b0399db
948846f
a251681
84361f6
50172bb
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My read of https://tailwindcss.com/docs/extracting-components#extracting-component-classes-with-apply is that this is where the
@apply
-ed stuff goes.But I thought we were already getting those in the compiled css? 🤔
Is it possible we're now duplicating anything from
@apply
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think Jin noted that the
components.css
class contains really few things: https://unpkg.com/[email protected]/dist/components.css. My read of that section was that you can use@layer
to control specificity, but it doesn't overlap with the content of@tailwind components
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And if the component classes are not being used, they should be purged out
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah you're right. Looks like
@apply
stuff gets inlined right into the .module.css file, not put incomponents
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hadn't thought of this until now, but another option is to allow utility classes in stories, but only use
@apply
in component styles. (And definitely allow utility classes in apps)Then there's no chance for conflicts and no unfortunate increase in size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or put another way:
I think the value in utility classes is in storybook stories and in an app. So we don't necessarily need to actually use them in the design system components if we don't want to.
(If we do want to then that's fine too)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense to me! It's essentially what we're doing already (since
global.css
is only used in the storybook file), but we could be more explicit by onlypurge
-ing story files. I'm less worried about the size since according to Tailwind,there is an argument that it's easier to just follow the standard convention though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Separated
components
andutilities
as suggested intotailwindUtilities.css
which is imported into.storybook/preview.tsx
for storybook use in edsLet me know if that's not proper naming conventions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may want to put the
@tailwind
stuff in another file. Something likesrc/styles/tailwind.css
.That way an App already using Tailwind (like Traject is/will?) can use it's own Tailwind stuff instead of importing 2 of them (one from here and one from the app), by only importing
src/styles/tailwind.css
if it's not already doing so.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can certainly rename this file. It currently only contains
tailwind
stuff, and it's actually not even imported intraject
because there were issues with including all of the base styles (traject defines its owntailwind.base.css
instead)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, okay. I assumed there was other stuff going on in it, but it makes sense that there's not.
Naming it
tailwind.css
sometime might make it more clear when to (and not to) include it in an app.