-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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(components): Add static class name with button style #26639
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.
Thanks @mskelton for the change. Given that we're changing the button class is there expected to be any UI changes? If so would you mind including before/after screenshots. |
@john-bodley This PR adds a new class, it doesn't change or remove any existing classes, so there are no expected UI changes. |
Seems superset-frontend/package-lock.json should not be affected by this PR, mind pulling it out? |
Can you explain the use case for the class? We're a little nervous about adding code that serves no purpose to most folks. If this is to support CSS templates at the dashboard level (where you can add custom CSS) that's one thing... we would just want to annotate the lines of code serving this purpose. If this is to customize the button for an entire Superset installation, the correct way would be to edit the Emotion based styling in the Button component, rather than adding hooks to layer on additional CSS files. CSS and LESS have been deprecated in favor of Emotion, and we'd rather see changes made on those grounds, or even steer toward more proper Emotion-based theming. |
@rusackas The purpose is to support custom CSS for embedded dashboards. The use case I have is to customize the controls in the dashboard (e.g. buttons, checkbox, dropdowns, etc. to align more closely with the app we are embedding superset in. Most controls I'm able to use class names from the Ant components (e.g. Happy to add a comment to the line explaining the purpose of the class name. |
@mistercrunch The package-lock.json was committed by mistake, I've reverted it. |
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
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #26639 +/- ##
==========================================
+ Coverage 69.04% 69.07% +0.02%
==========================================
Files 1931 1932 +1
Lines 75327 78860 +3533
Branches 8429 9511 +1082
==========================================
+ Hits 52013 54475 +2462
- Misses 21167 22056 +889
- Partials 2147 2329 +182
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
closing/reopening to kick CI. |
SUMMARY
Adds a static class name to buttons based on their style (e.g. primary, secondary, etc.) to more easily allow custom CSS styling of buttons.
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION