-
Notifications
You must be signed in to change notification settings - Fork 961
chore: upgrade to tailwind 4 #19247
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
base: main
Are you sure you want to change the base?
chore: upgrade to tailwind 4 #19247
Conversation
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 good for the most part! Just a few false positives and things that broke
window.addEventListener("blur-sm", closeOnTabSwitch); | ||
return () => window.removeEventListener("blur-sm", closeOnTabSwitch); |
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.
Codemod broke this
@@ -496,8 +496,8 @@ export const MultiSelectCombobox = forwardRef< | |||
<Badge | |||
key={option.value} | |||
className={cn( | |||
"data-[disabled]:bg-content-disabled data-[disabled]:text-surface-tertiarydata-[disabled]:hover:bg-content-disabled", | |||
"data-[fixed]:bg-content-disabled data-[fixed]:text-surface-tertiary data-[fixed]:hover:bg-surface-secondary", | |||
"data-disabled:bg-content-disabled data-[disabled]:text-surface-tertiarydata-[disabled]:hover:bg-content-disabled", |
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 only some of them got changed. There's also some broken spaces
@@ -91,7 +91,7 @@ export const Combobox: FC<ComboboxProps> = ({ | |||
<ChevronDown className="size-icon-sm text-content-secondary group-hover:text-content-primary" /> | |||
</Button> | |||
</PopoverTrigger> | |||
<PopoverContent className="w-[var(--radix-popover-trigger-width)]"> | |||
<PopoverContent className="w-(--radix-popover-trigger-width)"> |
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.
Does this work? I'm still seeing the var
syntax in some of the docs
https://tailwindcss.com/docs/theme#with-arbitrary-values
@@ -102,7 +102,7 @@ export const SelectContent = React.forwardRef< | |||
className={cn( | |||
"p-1", | |||
position === "popper" && | |||
"h-[var(--radix-select-trigger-height)] w-full min-w-[var(--radix-select-trigger-width)]", | |||
"h-(--radix-select-trigger-height) w-full min-w-(--radix-select-trigger-width)", |
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.
Flagging this var here, too
@@ -54,7 +54,7 @@ const TableFooter = React.forwardRef< | |||
<tfoot | |||
ref={ref} | |||
className={cn( | |||
"border-t bg-muted/50 font-medium [&>tr]:last:border-b-0", | |||
"border-t bg-muted/50 font-medium last:[&>tr]:border-b-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.
I think this is okay, but just making sure: did we test this one?
@@ -71,7 +71,7 @@ export const useAppLink = ( | |||
displayError(`${label} must be installed first.`); | |||
} | |||
}, openAppExternallyFailedTimeout); | |||
window.addEventListener("blur", () => { | |||
window.addEventListener("blur-sm", () => { |
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.
More broken non-CSS code
@@ -48,7 +48,7 @@ export const InboxPopover: FC<InboxPopoverProps> = ({ | |||
<InboxButton unreadCount={unreadCount} /> | |||
</PopoverTrigger> | |||
<PopoverContent | |||
className="w-[var(--radix-popper-available-width)] max-w-[466px]" | |||
className="w-(--radix-popper-available-width) max-w-[466px]" |
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.
Another var case
@@ -59,7 +59,7 @@ export const InboxPopover: FC<InboxPopoverProps> = ({ | |||
className={cn([ | |||
"[--bottom-offset:48px]", | |||
"[--max-height:calc(var(--radix-popover-content-available-height)-var(--bottom-offset))]", | |||
"[&>[data-radix-scroll-area-viewport]]:max-h-[var(--max-height)]", | |||
"*:data-radix-scroll-area-viewport:max-h-(--max-height)", |
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 okay, but I'm drawing attention to it, just in case: I don't know if the previous style was valid, since we didn't have the &>*
. I'm always skittish about fixing something that was previously broken, and having that ripple out
@@ -51,7 +51,7 @@ const DeploymentSettingsLayout: FC = () => { | |||
</BreadcrumbList> | |||
</Breadcrumb> | |||
<hr className="h-px border-none bg-border" /> | |||
<div className="px-10 max-w-screen-2xl"> | |||
<div className="px-10 max-w-(--breakpoint-2xl)"> |
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.
Surprised that Tailwind doesn't have classes for this
Most of the diff is from their upgrade tool doing code-mod stuff.
I think I'm gonna migrate the tailwind.config.js file to CSS config separately just to keep things reviewable and small.