feat(style): adding typographic variables to theme #463
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/superset/superset-ui/3yug0msbh |
rusackas
changed the title
Codecov Report
@@ Coverage Diff @@
## master #463 +/- ##
=======================================
Coverage 22.36% 22.36%
=======================================
Files 266 266
Lines 6549 6549
Branches 605 605
=======================================
Hits 1465 1465
Misses 5047 5047
Partials 37 37
Continue to review full report at Codecov.
|
| xl: '21px', | ||
| xxl: '28px', | ||
| }, | ||
| }, | ||
| gridUnit: '4px', |
There was a problem hiding this comment.
Can we make these numeric so possible future computation can be easier? I think it makes sense to make base units always be pixels.
There was a problem hiding this comment.
For example, the new react-select uses baseUnit arithmetics directly in custom styles:
There was a problem hiding this comment.
I've gone back and forth on this, and so have others in the discussions. The numeric bits do make things simpler in one sense, but being explicit about units in the theme also opens you up to having heterogeneous units, e.g. some things in rems or viewport units, or percentages, or whatever, without having to interpret them when looking at the theme file.
There was a problem hiding this comment.
But yes, I'll make the change... this is really just to support a little POC work... we can adjust as this thing grows.
There was a problem hiding this comment.
I guess the other downside is that we'd have to tack on the px in exponentially more places in the code (every instance of the style bing used), where it's centralized here. But we can try it out this way and see how it goes.
@ktmud Change is committed, if you don't mind approving, so I can work on the new POC PR.
There was a problem hiding this comment.
I’m in favor of using relative units whenever possible, but baseUnits should be pixels as the computation needs to start from somewhere. Which are categorized as baseUnits is debatable, but I’d imagine most of those in the core theme definition should be.
Another choice is to use CSS variables. Facebook just did that in their new website: https://engineering.fb.com/web/facebook-redesign/
kristw
changed the title
💔 Breaking Changes
🏆 Enhancements
Adding a few styles to the superset theme object, just to fiddle around with in other packages
📜 Documentation
🐛 Bug Fix
🏠 Internal