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

Storybook UI theming #3628

Merged
merged 118 commits into from
Jul 1, 2018
Merged

Storybook UI theming #3628

merged 118 commits into from
Jul 1, 2018

Conversation

ndelangen
Copy link
Member

@ndelangen ndelangen commented May 22, 2018

Issue: #2423 #3679

What I did

  • Consolidate UI components (WIP)
  • Add theme variables
  • implement a theme
  • implement a second theme
  • add ability to set theme from config.js

Also

  • Remove knobs from URL (still able to load from URL, but they get cleared)
  • Remove debounces from knobs input

Copy link
Member

@tmeasday tmeasday left a comment

Choose a reason for hiding this comment

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

Am I missing something? I don't see the themes or support for them via config.js in here.

This PR seems more about refactoring the addons component to use the active parameter, which seems fine in itself.

lib/addons/src/index.js Show resolved Hide resolved

storiesOf('Components|Tabs', module)
.addDecorator(s => <div style={{ height: 'calc(100vh - 20px)' }}>{s()}</div>)
.add('statefull - no initial', () => <TabsState panels={panels} />)
Copy link
Member

Choose a reason for hiding this comment

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

stateful

@ndelangen
Copy link
Member Author

@tmeasday Indeed, This PR so far is mainly consolidating, I'll try and consolidate some more components and then start adding theming to them.. It doesn't make a lot of sense to add theming in 60 places and then refactor then after I think.

I'll continue working on this PR, so there's more to come!

@tmeasday
Copy link
Member

Why don't we whack an "in progress" label on it so others like me don't get confused

@ndelangen
Copy link
Member Author

Ow I should have applied the label indeed, I prefixed my PR with "WIP" to indicate.

Still I love early reviews, so thanks, all feedback is appreciated!

@ndelangen
Copy link
Member Author

@tmeasday :

If the user is using es6 classes (as our docs current use) or a SFC, they may have panel.propTypes = { active: ... }. Especially if we tell them to in the new docs!.

We'd be forcing addon developers to adopt propTypes... Not everyone uses proptypes.

@ndelangen
Copy link
Member Author

@Hypnosphi thanks, I refactored the Tabs component, do you like it better this way?

@ndelangen
Copy link
Member Author

I'll try and fix this up over my trip home, and get it in a green state. Thanks for the review @Hypnosphi

render() {
return <Panel />;
},
// eslint-disable-next-line react/prop-types
Copy link
Member

Choose a reason for hiding this comment

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

Personally not a huge fan of these. Is there a reason?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm passing down a function, not a component..

Or is the comment regarding the switched based on active?

const props = {
actions: this.state.actions,
onClear: () => this.clearActions(),
};
return <ActionLoggerComponent {...props} />;
return active ? <ActionLoggerComponent {...props} /> : null;
Copy link
Member

Choose a reason for hiding this comment

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

Personal preference, add a conditional to return null instead of using a ternary.

Copy link
Member Author

Choose a reason for hiding this comment

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

What would your solution look like?

An if statement above?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah :)

if (!active) {
  return null;
}

return <ActionLoggerComponent {...props} />;

I do this because I can change the two conditions easier if needed and it makes it more readable imho.

@@ -8,7 +8,8 @@ export function register() {
const channel = addons.getChannel();
addons.addPanel(PANEL_ID, {
title: 'Action Logger',
render: () => <ActionLogger channel={channel} api={api} />,
// eslint-disable-next-line react/prop-types
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

@@ -27,7 +27,6 @@
"@storybook/addons": "4.0.0-alpha.10",
"@storybook/core-events": "4.0.0-alpha.10",
"babel-runtime": "^6.26.0",
"emotion": "^9.1.3",
Copy link
Member

Choose a reason for hiding this comment

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

When you remove emotion, what's left is abstract emptiness. Mah hart. Mah sole...

Copy link
Member Author

Choose a reason for hiding this comment

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

💔


if (!active) {
return null;
}
if (!backgrounds.length) return <Instructions />;
Copy link
Member

Choose a reason for hiding this comment

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

Newline between statements, and curly braces? Not sure what the rules here are.

Copy link
Member

Choose a reason for hiding this comment

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

If eslint passes then it's OK

@ndelangen ndelangen merged commit 33de29e into master Jul 1, 2018
@ndelangen ndelangen deleted the feature/theming branch July 1, 2018 11:28
@shilman shilman changed the title Feature/theming Storybook UI theming Jul 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants