-
Notifications
You must be signed in to change notification settings - Fork 1.1k
feat: remove css from various components/ elements
#21263
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?
Conversation
| color: theme.roles.warning.text, | ||
| }), | ||
| } satisfies Record<string, Interpolation<Theme>>; | ||
| const badgeClasses = { |
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.
Not sure if we want to do this as apart of this PR but this probably a good candidate for class-variance-authority.
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 already have a dependency on it, go ahead
| return ( | ||
| <span | ||
| css={(theme) => ({ color: theme.palette.text.secondary, fontSize: 13 })} | ||
| className={"text-[13px] text-content-secondary"} |
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.
why set this to 13px instead of using a utility class?
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.
text-xs is 12px and text-sm is 14px. Neither really seemed to fit the vibe this was going for, YOMV tho. I might revert this as you're redoing Alert.tsx in another PR 🙂
| { color: theme.palette.text.secondary }, | ||
| ]} | ||
| > | ||
| <div className="text-sm leading-relaxed text-zinc-600 dark:text-zinc-400"> |
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 could just update the colors now by looking up the component in Figma, https://www.figma.com/design/WfqIgsTFXN2BscBSSyXWF8/Coder-kit?node-id=711-383&t=WnARy2Vp9NARG6V0-1
So no need to set a dark variant as that will be handled by the Tailwind color
| className={cn( | ||
| "flex flex-row flex-nowrap items-center gap-4", | ||
| "p-4 rounded-lg cursor-default", | ||
| "border border-solid border-zinc-200 dark:border-zinc-700", |
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.
Colors can be found in figma, https://www.figma.com/design/WfqIgsTFXN2BscBSSyXWF8/Coder-kit?node-id=711-383&t=WnARy2Vp9NARG6V0-1
| "px-3 rounded-full flex items-center w-fit whitespace-nowrap", | ||
| "border border-solid leading-none", | ||
| ], | ||
| enabled: ["border-green-500 bg-green-950 text-green-50"], |
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 we update these colors from the theme colors but don't use colors from tailwind.config.js, we may forget to do it later. If you feel like the proper colors don't yet exist in tailwind.config, we should atleast have an issue created to cleanup
| const classNames = { | ||
| root: [ | ||
| "flex flex-row justify-center items-center min-h-[280px] p-6 rounded-lg gap-8", | ||
| "bg-[linear-gradient(160deg,_transparent,_rgb(46,_16,_101))]", | ||
| "border border-solid border-violet-400", | ||
| ].join(" "), | ||
| feature: "flex items-center p-[3px] gap-2", | ||
| }; |
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.
Any reason why this is necessary? with Tailwind its generally best practice to inline the classes even if there is duplication.
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.
Honestly I wanted to rely harder on the JIT translation but as we're on Tailwind v3 right now some things are being done manually. I can either document here that this is an opportunity to update after an upgrade to v4 or inline, thoughts?
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 don't think it makes a difference for JIT translation if the tailwind is supplied inline or a separate variable, it just matters if it is constructed dynamically. I would still prefer to keep everything inline and use proper methods for dynamic classnames, css variables, etc.
| })} | ||
| className={cn( | ||
| "py-[1px] px-1 rounded-sm text-sm leading-none text-content-primary", | ||
| "bg-surface-tertiary dark:bg-surface-quaternary", |
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 shouldn't ever have to set dark when using colors from tailwind.config, either the colors should change or we are missing colors
| {...props} | ||
| className={cn([ | ||
| "py-[1px] px-1 rounded-sm text-sm leading-none", | ||
| "bg-surface-tertiary dark:bg-surface-quaternary", |
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 shouldn't ever have to set dark when using colors from tailwind.config, either the colors should change or we are missing colors
| color: theme.roles.warning.text, | ||
| }), | ||
| } satisfies Record<string, Interpolation<Theme>>; | ||
| const badgeClasses = { |
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 already have a dependency on it, go ahead
| <HelpTooltipIcon css={{ color: iconColor }} /> | ||
| <HelpTooltipIconTrigger | ||
| size="small" | ||
| className="opacity-100 hover:opacity-100" |
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.
| className="opacity-100 hover:opacity-100" | |
| className="opacity-100" |
|
|
||
| return ( | ||
| <Topbar css={{ gridArea: "topbar" }}> | ||
| <Topbar className="flex" style={{ gridArea: "topbar" }}> |
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.
where's this flex coming from?
Co-authored-by: ケイラ <mckayla@hey.com>
Co-authored-by: ケイラ <mckayla@hey.com>
Co-authored-by: ケイラ <mckayla@hey.com>
Co-authored-by: ケイラ <mckayla@hey.com>
This attempts to remove all of the MUI/
@emotionbasedcssprops. We've begun by doing this specifically in thecomponents/folder. This should serve to align ourselves better with Tailwind/shadcn and our goal to remove mui from the database.I've made attempts to ensure the
line-heightis consistent, however this isn't always in best-practice with Tailwind in some cases. Minor pixel shifts may happen. There are around 9 or so components living incomponents/I haven't updated yet as the logic was scarier to migrate away from MUI.Note
This pull-request is a fork-out of #21154 as I found it was getting too big to merge and hard to understand. Alongside this various components were being misrepresented in Chromatic (despite rendering fine in Storybook). I felt it better to make this a set of PRs.
Chromatic Link / Chromatic Tests
cssfrom variouscomponents/elementscssfrom variouspages/Template.*pages