Skip to content
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

Closed
wants to merge 9 commits into from
Closed

Theming support #428 #1360

wants to merge 9 commits into from

Conversation

usulpro
Copy link
Member

@usulpro usulpro commented Jun 26, 2017

issue: #428

What I did

1. Add new mantra module to @storybook/ui (src/modules/theming/) to provide theming API:

  • selectTheme(themeName: string) selects the current theme
  • getTheme(): object returns current theme object
  • getThemesList(): arrayOf(strings) returns names of loaded themes
  • setTheme(theme: object) adds new or updates existing theme

2. Add public API defaultState.uiOptions.currentTheme to @storybook/ui

available as:

import { setOptions } from '@storybook/addon-options';

setOptions({currentTheme: 'default'});

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 a theme prop.

5. Make all style... objects depend on the theme data

6. 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 the config.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 by setOptions({currentTheme: 'dark'});

3. In order to test mantra module API you can do follow steps:

  • uncomment the line in src/modules/theming/index.js

console.log('UI API:', _actions);

  • in chrome console: right click on 'UI API:' and 'Store as GlobalVariable'
  • api = temp1;
  • api.theming.getTheme();
  • api.theming.selectTheme('dark');
  • newTheme = api.theming.getTheme();
  • newTheme.name = 'new';
  • api.theming.setTheme(newTheme);
  • api.theming.getThemesList();
  • api.theming.selectTheme('new');
  • newTheme.palette.canvas = 'green';
  • newTheme.palette.background = 'darkgreen';

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:

  • creation new built-in themes or improving existing ones
  • rethink what styles/data should be included in theme objects
  • provide a pluggable way to add new custom themes

@codecov
Copy link

codecov bot commented Jun 26, 2017

Codecov Report

Merging #1360 into release/3.3 will decrease coverage by 4.14%.
The diff coverage is 22.22%.

Impacted file tree graph

@@               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
Impacted Files Coverage Δ
lib/ui/src/modules/api/index.js 0% <ø> (ø) ⬆️
lib/ui/src/modules/theming/configs/init_themes.js 0% <0%> (ø)
lib/ui/src/modules/theming/actions/theming.js 0% <0%> (ø)
lib/ui/src/modules/theming/index.js 0% <0%> (ø)
lib/ui/src/modules/theming/themes/default.js 0% <0%> (ø)
lib/ui/src/index.js 0% <0%> (-13.34%) ⬇️
lib/ui/src/modules/ui/components/search_box.js 0% <0%> (-16.67%) ⬇️
lib/ui/src/modules/theming/actions/index.js 0% <0%> (ø)
lib/ui/src/modules/theming/themes/dark.js 0% <0%> (ø)
...b/ui/src/modules/ui/components/down_panel/style.js 100% <100%> (ø)
... and 139 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a2802e5...6844098. Read the comment docs.

@danielduan
Copy link
Member

danielduan commented Jun 26, 2017

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.

  1. Theming the Storybook Panel UI to match product branding
    (My company uses white text on navy background, and I want the UI panels to be white and navy as well.)
  • It looks like we are taking care of that with the setOptions() addon we can use globally.
  1. Theming the story components and displaying them in various backgrounds
    (I have a component that can be displayed on white background and dark background)
  • It seems a bit much to use setOptions to change the UI panel colors along with the story background every time.
  • There also doesn't seem to be an easy way to pass a prop or context to the component itself for the theme change. I would have to manually define each dark theme story and component.
  • There's no way to keep a theme toggled. Say I have a set of form components that all should be themable in light and dark. I'd have to select the kind and the dark theme story each time to view all the components in dark.
  • Components with many stories are hard to theme. If I have a Button that has (warning, accept, cancel) stories, I would need to make (warning:light, warning:dark, accept:light, accept:dark, ...).
  • Components have to be manually enabled to view in a certain theme. Say I have a Button designed for light background and I want to see how it looks in dark, I'd have to go into the code to enable it. This isn't very friendly for project stakeholders who don't code (product managers, designers, etc).

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.

@usulpro
Copy link
Member Author

usulpro commented Jun 26, 2017

@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 config.js to set appropriate theme once for all storybook.

At this moment I created only one additional dark theme, and I'm looking for the best way to add API for providing users theme as well. We can do it via setOptions or by some separate API - happy to discuss the most convenient way.

Theming the story components (second use case) not related directly to this PR and there're some other means for that:

Copy link
Member

@danielduan danielduan left a 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%);*/
Copy link
Member

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

@shilman
Copy link
Member

shilman commented Jun 27, 2017

@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

@usulpro
Copy link
Member Author

usulpro commented Jun 27, 2017

@shilman sure! This PR is focused on theming API for @storybook/ui and I tried to add only the minimal changes to UI components.

Copy link
Member

@danielduan danielduan left a 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,
Copy link
Member

@igor-dv igor-dv Jul 1, 2017

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 ?

Copy link
Member Author

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;
Copy link
Member

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 
/**/

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@usulpro usulpro force-pushed the theming-support-#428 branch from 16b959b to 93c6c14 Compare July 3, 2017 16:24
@ndelangen ndelangen mentioned this pull request Jul 7, 2017
Copy link
Member

@ndelangen ndelangen left a 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.

@ndelangen
Copy link
Member

Sorry my review took so long @usulpro

Will you help me transition the codebase to themable glamorous components?

@Hypnosphi
Copy link
Member

Hypnosphi commented Aug 6, 2017

Well, glamorous accepts themes as objects, so this approach should work with it as well. I think for now we can just move theme/theming stuff to lib/components/theming to indicate that that's where we go.

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

@Hypnosphi
Copy link
Member

Hypnosphi commented Aug 6, 2017

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

@usulpro
Copy link
Member Author

usulpro commented Aug 6, 2017

@ndelangen I will certainly help you with the transition to Glamorous and make sure that it will be themeable 😸
Where can we start from?

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 <ThemeProvider /> will be much more painless than how it's now.
So I think as soon we have any small piece of glamorous in Storybook, we can start to adopt this PR to work with them as per @Hypnosphi

@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.
Having white preview area with dark panel looked too contrasting for me (IMHO), but for sure we can rethink how it should look and I guess there will be a lot of work to make it better

@ndelangen
Copy link
Member

@usulpro how do you see this PR going forward?

@usulpro
Copy link
Member Author

usulpro commented Sep 7, 2017

@ndelangen I want to keep UI API to switch themes as a mantra module and move all themes into components. Then we'll wrap everything into Glamorous ThemeProvider.
Do you agree with this approach?

@ndelangen ndelangen closed this Sep 24, 2017
@usulpro usulpro changed the base branch from release/3.2 to release/3.3 September 24, 2017 08:36
@usulpro usulpro reopened this Sep 24, 2017
@danielduan
Copy link
Member

Closing this because there hasn't been any activity here for a while. Feel free to reopen when this is ready for discussion again.

@danielduan danielduan closed this Nov 14, 2017
@sum32
Copy link

sum32 commented Dec 27, 2017

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 .storybook dir create a file called manager-head.html and just put this style tag inside(with some more css classes this kind of solution would work for more usecases):

<style type="text/css">
    .Pane1{
        background-color: #2A2A2E;
        color: white;
        font-size: 20px !important;
    }

    .Pane1 div{
        font-size: 25px !important;
        line-height: 1em;
    }

    .Pane1 a{
        background-color: #2A2A2E;
        font-size: 20px !important;
    }

    .Resizer{
        background-color: #2A2A2E;
    }

    .Pane1 input{
        background-color: #2A2A2E !important;
        color: white !important;
        font-size: 20px !important;
    }

    .Pane2{
        background-color: #2A2A2E;
        color: white;
    }

    .Pane2 div{
        background-color: #2A2A2E;
        font-size: 20px;
    }

    .Pane2 ol {
        background-color: #2A2A2E;
        font-size: 20px;
    }

    /* text in action logger object*/
    .Pane2 span{
        padding: 1em 0 0 0 !important;
        color: cyan !important;
    }

    .Pane2 span > span:first-child{
        color: white !important;
    }

</style>

@Hypnosphi Hypnosphi deleted the theming-support-#428 branch February 17, 2018 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants