-
Notifications
You must be signed in to change notification settings - Fork 119
rm postcss #2901
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
rm postcss #2901
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| maxHeight?: string; | ||
| fixed?: boolean; | ||
| } | ||
| class?: string; |
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 the idea is to use the ClassValue type here, which supports arrays, objects, etc. but will defer to @GiantRobots
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.
ClassNameValue from the twmerge import is what we want (twmerge supports all of the cslx goodness... except objects. So we want to use what their api allows so we don't have to do more work subbing out to cslx, then merge, etc. objects are fine to not use because you can use arrays and you basically just invert the declarations)
| type Item = $$Generic; | ||
| interface $$Props extends HTMLAttributes<HTMLTableElement> { | ||
| type $$Props = HTMLAttributes<HTMLTableElement> & { |
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 the change from interface to type?
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 was trying to switch from legacy over to runes and then decided to go back when it went bad and there were apparently casualties along the way.
| updating?: boolean; | ||
| maxHeight?: string; | ||
| fixed?: boolean; | ||
| class?: ClassValue; |
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.
Use ClassNameValue from twmerge
| class={merge( | ||
| 'surface-primary min-h-[154px] grow overflow-auto border border-subtle', | ||
| className, | ||
| )} |
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.
Just use the regular merge with the different type on classes and we gucci
| "@codemirror/lang-go": "^6.0.1", | ||
| "@codemirror/lang-java": "^6.0.2", | ||
| "@codemirror/lang-json": "^6.0.1", | ||
| "@codemirror/lang-php": "^6.0.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.
Probably don't need these codemirror addons?
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 they just got alphabetized
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.
Yeah, this happened when I ran install for some reason. But Those aren't new additions.
GiantRobots
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.
lgtm
Description & motivation 💭
Switch from postcss to twmerge for classes in the PaginatedTable component.
Does this seems like a better solution for now?
I didn't switch to away from legacy because of all of the different types of snippets we will probably need to add for this. And we will need to update all of the use-cases for this component. I think we will probably want to do that in the different ticket if we choose to do that? Just being we need to update both repos. I want to make sure we don't break any pages with the update.
Screenshots (if applicable) 📸
Design Considerations 🎨
Testing 🧪
How was this tested 👻
Steps for others to test: 🚶🏽♂️🚶🏽♀️
Checklists
Draft Checklist
Merge Checklist
Issue(s) closed
Docs
Any docs updates needed?