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
Replace classnames with clsx #61138
Replace classnames with clsx #61138
Conversation
@dcalhoun FYI the bundle size comment finally got posted 🎉 #61138 (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 👍 Left a few minor suggestions/questions, but nothing critical that can't be addressed in a follow-up PR.
const classes = classnames( | ||
'components-focal-point-picker__icon_container' | ||
); | ||
const classes = clsx( 'components-focal-point-picker__icon_container' ); |
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.
For some of those, I wonder why we even need a classnames/clsx utility. Guess it can be removed.
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.
True, this one's honestly kind of funny lol. Guessing there was a point in using it before and then it was updated and they just didn't remove the classnames
call.
@@ -94,8 +94,7 @@ export const SVG = forwardRef( | |||
const appliedProps = { | |||
...props, | |||
className: | |||
classnames( className, { 'is-pressed': isPressed } ) || | |||
undefined, | |||
clsx( className, { 'is-pressed': isPressed } ) || undefined, |
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 know it was here before, but do we really need the || undefined
?
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 makes no sense. Honestly I didn't really look into these, I just search-replaced.
It looks like this PR is the first one making a change to the I tried rolling that back the update to actions/setup-node, and and it worked for me (#61289). I also moved the debugging steps we added above to that PR to keep this one specific to the task at hand. |
Thanks @desrosj, I'm gonna remove the unrelated CI debug changes in order to merge the PR. |
…gnore/clsx-size-test
PR originally started as a test, see: #61091 (comment)
Then it replaced #61091, see: #61091 (comment)
There are no significant changes in this PR that weren't already part of the previous PR, I only synced with trunk and ensured that all usage of classnames was still consistently replaced with cslx.