-
Notifications
You must be signed in to change notification settings - Fork 77
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
refactor: Upgrade to tailwind version 3 #3700
Conversation
# Conflicts: # package-lock.json # package.json
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.
Initial comments.
future: { | ||
removeDeprecatedGapUtilities: true | ||
}, | ||
variants: { |
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.
@@ -259,13 +270,6 @@ module.exports = { | |||
const utilities = Object.assign({}, ...colorMap); | |||
|
|||
addUtilities(utilities, variants("borderColor")); | |||
} | |||
], | |||
future: { |
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.
@@ -33,5 +33,5 @@ | |||
} | |||
|
|||
.menu { | |||
@apply flex-col flex-no-wrap outline-none; | |||
@apply flex-col flex-nowrap outline-none; |
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.
@@ -39,7 +39,7 @@ | |||
::slotted(calcite-action-group) { | |||
@apply border-0 | |||
p-0; | |||
border-inline-end-width: theme("borderWidth.default"); | |||
border-inline-end-width: theme("borderWidth.DEFAULT"); |
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.
@@ -56,8 +56,7 @@ | |||
} | |||
|
|||
.tip-container { | |||
@apply mt-0 |
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.
removed mt-0
because mt-px;
was already setting the margin.
@@ -21,7 +21,7 @@ calcite-pick-list-item { | |||
|
|||
:host([active]), | |||
:host([selected]) { | |||
@apply shadow-outline-active; | |||
@apply ring; |
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.
tailwind.config.ts
Outdated
@@ -44,7 +45,14 @@ module.exports = { | |||
2: "var(--calcite-ui-text-2)", | |||
3: "var(--calcite-ui-text-3)", | |||
inverse: "var(--calcite-ui-text-inverse)", | |||
link: "var(--calcite-ui-text-link)" | |||
link: "var(--calcite-ui-text-link)", | |||
color: { |
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 had to add this here to get it to find text-color-{x}
. If someone knows if we can restructure this somehow that would be great. :)
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.
If this doesn't get resolved in this PR, can you create a follow-up issue for this (assuming we'd want to change it)?
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.
Seems like something is going on with the font sizing and line height. Can someone assist with that part? |
@@ -18,7 +18,7 @@ | |||
justify-start | |||
m-0 | |||
relative | |||
text--2h | |||
text-n2h |
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.
classes that start with a dash were not working. They would either not be applied or cause a build error.
I changed all of them to be n2h
instead of -2h
.
# Conflicts: # src/components/calcite-panel/calcite-panel.scss # src/components/calcite-tab-nav/calcite-tab-nav.scss
@@ -21,7 +21,7 @@ | |||
justify-end | |||
items-stretch | |||
mb-1; | |||
&.sticky { | |||
&.sticky-pos { |
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.
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.
Looks like we need to make sure our classes never match tailwind utils. We could look into creating a stylelint rule or similar to prevent circular dependencies.
All tests passing, please review. |
@@ -30,5 +30,5 @@ h3.heading { | |||
} | |||
h4.heading, | |||
h5.heading { | |||
@apply text--1h; | |||
@apply text-n1h; |
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.
classes that start with a dash were not working. They would either not be applied or cause a build error.
I changed all of them to be n2h instead of -2h.
@@ -68,7 +68,7 @@ | |||
} | |||
|
|||
.image-frame { | |||
width: theme("width.[1/4"); |
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.
This one was missing a closing bracket. I'm not sure it was failing but maybe we can use some linting here.
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.
Awesome! Thanks for working on this to bump Tailwind to the latest version. 🎉
I'll defer to @macandcheese and/or @asangma on some comments, but the code LGTM!
@@ -21,7 +21,7 @@ | |||
justify-end | |||
items-stretch | |||
mb-1; | |||
&.sticky { | |||
&.sticky-pos { |
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.
Looks like we need to make sure our classes never match tailwind utils. We could look into creating a stylelint rule or similar to prevent circular dependencies.
@@ -68,7 +68,7 @@ | |||
} | |||
|
|||
.image-frame { | |||
width: theme("width.[1/4"); | |||
width: theme("width[1/4]"); |
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.
Nice catch!
tailwind.config.ts
Outdated
@@ -44,7 +45,14 @@ module.exports = { | |||
2: "var(--calcite-ui-text-2)", | |||
3: "var(--calcite-ui-text-3)", | |||
inverse: "var(--calcite-ui-text-inverse)", | |||
link: "var(--calcite-ui-text-link)" | |||
link: "var(--calcite-ui-text-link)", | |||
color: { |
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.
If this doesn't get resolved in this PR, can you create a follow-up issue for this (assuming we'd want to change it)?
# Conflicts: # package-lock.json # package.json # src/components/calcite-shell-center-row/calcite-shell-center-row.scss # stencil.config.ts
# Conflicts: # package-lock.json # package.json # stencil.config.ts
}, | ||
({ addUtilities, theme, variants }) => { | ||
}), | ||
plugin(({ addUtilities, theme, variants }) => { |
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'm not sure if these plugins are still needed. Tailwind 3 might provide these already.
@@ -109,10 +110,7 @@ module.exports = { | |||
l: "1024px", | |||
xl: "1440px" | |||
}, | |||
textColor: (theme) => ({ |
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.
This wasn't necessary anymore.
color: theme("colors.text") | ||
}), | ||
backgroundColor: (theme) => ({ | ||
backgroundColor: ({ theme }): object => ({ |
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 might be able to remove or refactor this at some point.
@@ -147,9 +145,6 @@ module.exports = { | |||
"outline-active": "0 0 0 1px var(--calcite-ui-brand)", | |||
none: "none" | |||
}, | |||
fill: (theme) => ({ |
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.
Wasn't necessary anymore
@@ -39,7 +40,7 @@ module.exports = { | |||
3: "var(--calcite-ui-foreground-3)" | |||
} | |||
}, | |||
text: { | |||
color: { |
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 might be able to remove or refactor this at some point. Tailwind colors usually just go like text-<color>
instead of text-color-<color>
. This allows people to use colors as variants
Related Issue: #2078
Summary
refactor: Upgrade to tailwind 3
Issues to track: