-
Notifications
You must be signed in to change notification settings - Fork 1.1k
refactor(site): convert workspace batch delete dialog to Tailwind CSS #20946
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(site): convert workspace batch delete dialog to Tailwind CSS #20946
Conversation
Add comprehensive documentation for active frontend pattern migrations discovered through analysis of 200+ git commits touching site/ files: Core Migrations: - Emotion → Tailwind CSS (strict 'no new emotion' policy) - MUI Components → Custom/Radix/shadcn (Tooltips, Tables, Buttons) - MUI Icons → lucide-react with specific icon mappings - spyOn → queries parameter for GET endpoint mocks in Storybook - localStorage → user_configs table for user preferences New Documentation: - Icon migration mappings (BusinessIcon→Building2Icon, etc.) - Radix component prop naming conventions (placement→side) - cn() utility usage for conditional className merging - Chromatic testing best practices (prefer snapshots over assertions) Includes concrete before/after examples and migration patterns to guide developers away from deprecated approaches toward current best practices. Analysis based on PRs: #20948, #20946, #20938, #20905, #20900, #20869, #20849, #20808, #20530, #20479, #20261, #20201, #20200, #20193, #20318 --- 🤖 This change was written by Claude Sonnet 4.5 Thinking using mux and reviewed by a human 🏂
|
@mafredri its always nice to have a screenshot in the PR description, especially for styling updates like this |
| return ( | ||
| <> | ||
| <ul css={styles.workspacesList}> | ||
| <ul className="list-none p-0 border border-solid border-zinc-200 dark:border-zinc-700 rounded-lg overflow-x-hidden overflow-y-auto max-h-[184px]"> |
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 should never have to use a specific color like border-zinc-200, all colors should come from tailwind.config.js and dark will be automatically handled. For borders it should most likely be, border border-border border-solid If this is indeed a special case where the existing border color is not acceptable, then a new color would be added to tailwind.config.js and index.css.
Also for max-h-[184px], if it still looks decent, we are trying to move from setting specific pixel widths and heights and use the closest utility class size that works.
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.
Gotcha, I did this to replicate the existing UI 1-to-1, I can simplify but it'll result in visual changes (border will be very faded).
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.
The colors have actually changed with the new designs, its useful to look at the Coder Kit design system in Figma, https://www.figma.com/design/WfqIgsTFXN2BscBSSyXWF8/Coder-kit?node-id=0-1&t=Epp3AXbEEYL9OGsB-1
So generally you wont't want to replicate the existing UI.
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's fair, I'll keep this in mind too. (Just to be clear, implementing the new design is not expected right? It's beyond the scope of this PR and not something I have bandwidth for.)
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 its not too involved, I generally update the styles during the emotion to Tailwind conversion. In most cases the design is almost the same. Typically its just updating to the new colors and removing the Stack component, if more is involved then its probably a separate issue/PR.
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.
The reason I asked is that I saw the batch dialogue had a big revamp in look/flow. I can remove Stack if preferred.
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 have another PR coming that will mark MUI and Stack as deprecated, #20973
The frontend team decided that the Stack component doesn't make much anymore with Tailwind.
Feel free to remove now, or we can get to it later.
jaaydenh
left a comment
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.
Avoid using border-zinc-200 dark:border-zinc-700
The UI didn't change, so I didn't think of it, I'll keep in mind in future. 👍🏻
👍🏻 |
…ialogue References #20905
…lete dialogs - Replace border-zinc-200 dark:border-zinc-700 with border-border - Replace max-h-[184px] with max-h-48 utility class - Add cancel button to workspaces batch delete dialog
80670f4 to
a326dc1
Compare
|
Note: I matched cancel button from tasks dialogue to be shown at every step, there was some confusion of how to escape this dialogue, and I see no harm in adding it in. |
Converts from Emotion to Tailwind CSS, based on the tasks batch delete dialog implementation.
Also propagates simplifications back to the tasks dialog:
border-borderinstead of hardcoded color variantsmax-h-48instead of specificmax-h-[184px]Refs #20905
Before:

After:

Before:

After:
