-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
feat: #3798 Replaced classnames with clsx #4632
Conversation
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.
✅ This pull request was sent to the PullRequest network.
@yuki0410-dev you can click here to see the review status or cancel the code review job.
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.
PullRequest Breakdown
Reviewable lines of change
+ 30
- 39
91% JavaScript
6% JavaScript (tests)
3% JSON
Generated lines of change
+ 5
- 5
Type of change
Feature - These changes are adding a new feature or improvement to existing code.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4632 +/- ##
=======================================
Coverage 96.96% 96.96%
=======================================
Files 28 28
Lines 2607 2607
Branches 1101 1101
=======================================
Hits 2528 2528
Misses 79 79 ☔ View full report in Codecov by Sentry. |
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.
This change does what the description says - no security issues here.
Reviewed with ❤️ by PullRequest
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.
These changes look good to me as well for upgrading to the more modern clsx
. I didn't notice any issues with the upgrade.
At a quick glance, I did notice here that there is a globals entry for classnames
, but provided that all package.json scripts are still running successfully after this upgrade, I don't think that should be addressed in this PR.
Nice work with this one! Another great improvement.
Reviewed with ❤️ by PullRequest
Thanks for the review. I removed classnames from global in rollup.config.mjs. |
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.
Thanks this looks good. |
Description
Linked issue: close #3798
Problem
See issue #3798
Changes
Replaced classnames with clsx
Screenshots
To reviewers
Contribution checklist