-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
Theming support #428 #1360
Theming support #428 #1360
Conversation
Codecov Report
@@ Coverage Diff @@
## release/3.3 #1360 +/- ##
===============================================
- Coverage 15.82% 11.68% -4.15%
===============================================
Files 237 210 -27
Lines 5043 4726 -317
Branches 711 512 -199
===============================================
- Hits 798 552 -246
- Misses 3659 3724 +65
+ Partials 586 450 -136
Continue to review full report at Codecov.
|
Thanks for doing this! The code looks good, but I'm a little concerned about what the use cases. It seems like we are mixing different needs from users into a feature that should probably be split.
What you're doing with setOptions is awesome for case 1. For case 2, I think a custom theme decorator of some sort that displays a persisting dropdown menu would work better. Again, really appreciate this. I've been wanting this feature for a while but never had the time to implement it. |
@danielduan Thank you for the quick response! Sorry If I didn't clarify it before: This PR covers only Storybook own appearance. So it's the first use case. It customizes all Storybook UI panels and the preview area (when it's empty like in the demo). And I suppose it could be mostly used in At this moment I created only one additional Theming the story components(second use case) not related directly to this PR and there're some other means for that:
|
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.
Sounds good! One last thing, can we update the README in addon-options
and add something about this change?
background: rgba(232,232,232,1); | ||
background: radial-gradient(ellipse at center, rgba(255,255,255,1) 11%,rgba(232,232,232,1) 100%); | ||
background: rgba(232,232,232,0); | ||
/*background: radial-gradient(ellipse at center, rgba(255,255,255,1) 11%,rgba(232,232,232,1) 100%);*/ |
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.
if this isn't needed, it's probably best to remove
@usulpro awesome!! 🙌 definitely coordinate with @ndelangen on this. He has something in mind for storybook components and it would be good if your change and his were compatible |
@shilman sure! This PR is focused on theming API for |
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 good man!
@@ -1,10 +1,11 @@ | |||
import { baseFonts } from '../theme'; | |||
|
|||
export default { | |||
export default theme => ({ | |||
empty: { | |||
flex: 1, | |||
display: 'flex', | |||
...baseFonts, |
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.
shouldn't base font be in the theme ?
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.
✅
@@ -4,9 +4,9 @@ import style from './style'; | |||
|
|||
class DownPanel extends Component { | |||
renderTab(name, panel) { | |||
let tabStyle = style.tablink; | |||
let tabStyle = style(this.props.theme).tablink; |
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.
maybe you'll define the style once, instead of creating it in each call ?
something like
import commonStyle from './style';
/**/
const style = commonStyle(this.props.theme);
// in this case you will have less conflicts and irrelevant changes
/**/
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.
✅
16b959b
to
93c6c14
Compare
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 theming specifically, I'd rather do this in a different way.
I think trying to solve the theming issue before fully addressing the styling + modularisation issue we have will do us more bad than good.
I want our UI to move into a css-in-js components library. Glamorous is my pick. And it supports Themeing: https://glamorous.rocks/advanced#theming
So I think that's how we should solve it.
In the end - We should allow allow users to provide their own custom components and custom theme.
Sorry my review took so long @usulpro Will you help me transition the codebase to themable glamorous components? |
Well, glamorous accepts themes as objects, so this approach should work with it as well. I think for now we can just move In fact, when I got started with "Glamorous components" task, the first thing I did was beginning to extract the common colors into a theme =) I wish I had seen this PR by then |
As for the dark theme: it looks really cool, but I don't think it's a good idea to change preview background, with the reasons given by @danielduan Of course, you can use addons background, but you can use it to set dark background as well, and in former case you can decide when your particular story is ready for it |
@ndelangen I will certainly help you with the transition to Glamorous and make sure that it will be themeable 😸 Actually, when I created this PR I meant switching to Glamourous Components so this API is more about to provide and change themes than current theme implementation. We can change the theme object so that it more suited to our needs. And passing theme data ones via @Hypnosphi 👍 I see what you mean. At the moment there is a mess with colors and styles used in UI. It could be nice to rearrange it somehow. |
@usulpro how do you see this PR going forward? |
@ndelangen I want to keep UI API to switch themes as a mantra module and move all themes into |
Closing this because there hasn't been any activity here for a while. Feel free to reopen when this is ready for discussion again. |
If you are like me and just want some dark colors and a bit bigger fontsize for the ui try this kind of hack, in your
|
issue: #428
What I did
1. Add new mantra module to
@storybook/ui
(src/modules/theming/
) to provide theming API:2. Add public API
defaultState.uiOptions.currentTheme
to@storybook/ui
available as:
3. Consolidate all UI colors style settings in
src/modules/theming/themes
default.js
- the current UI style (not changed)dark.js
- dark UI style (the new one)4. Pass theme data to all UI components via
react-komposer
(src/modules/ui/libs/gen_podda_loader.js
). So it updates automatically on any change. UI Components can access it as atheme
prop.5. Make all
style...
objects depend on the theme data6. Create "Dark" theme, while "Default" theme keeps current appearance.
How to test
1. Github pages: https://usulpro.github.io/storybook-themable
(here is an example of dynamic theme switching. We can use
setOptions
in theconfig.js
as well)2. Checkout
theming-support-#428
branch and:$ npm install $ npm run bootstrap $ cd examples/cra-kitchen-sink $ npm run storybook
you can set
currentTheme
bysetOptions({currentTheme: 'dark'});
3. In order to test mantra module API you can do follow steps:
src/modules/theming/index.js
console.log('UI API:', _actions);
What next
The aim of this PR is mainly to provide the API for theming support and pass (UI library agnostic) theme data to UI components. By adding this we're stepping towards to: