- Notifications
You must be signed in to change notification settings - Fork913
chore: replace MUI icons with Lucide icons - update 18#17958
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
BrunoQuaresma commentedMay 20, 2025
- PersonOutline → UserIcon
- Apps → LayoutGridIcon
- Delete → TrashIcon
- InsertDriveFile → FileIcon
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.
Changes look straightforward, so I'm approving to make sure you're not blocked. I had some concerns about the use ofTagIcon for some of the stories, but I'll let you decide whether that's something to worry about right now
Like I mentioned in the Storybook comments, there's a good possibility that the concept of tags will be introduced to the Coder UI in less than 2 months, so I'd prefer it if we didn't use the icon for non-tag elements, just to make sure we don't confuse the user
| <FileIcon | ||
| className="size-icon-xs" | ||
| css={{ | ||
| opacity: 0.5, | ||
| mr: 0.25, | ||
| marginRight: "0.25rem", |
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.
Genuine question: Is there a reason why this wasn't switched over to Tailwind syntax? I know some of the icon libraries can be a little finicky with styling
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.
Actually, wait, does this work as intended? I know that when you give a component acss prop, Emotion injects it viaclassName. Does Emotion take the existingclassName and merge it with its version ofclassName, or does it override the value?
| fontSize: 14, | ||
| <LayoutGridIcon | ||
| className="size-icon-xs" | ||
| css={{ |
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 potential area wherecss can be removed
@Parkreiner let me check the stories, but the WorkspaceAppStatus component, will be refactored soon 🙏 and maybe, removed. |
e1934fe intomainUh oh!
There was an error while loading.Please reload this page.