Storybook UI theming #3628
Storybook UI theming #3628
Conversation
…clude a `active` prop
# Conflicts: # lib/components/src/layout/__snapshots__/index.stories.storyshot
ndelangen
requested review from
alexandrebodin,
alterx,
Hypnosphi,
igor-dv,
jbovenschen,
leonrodenburg,
renaudtertrais,
rhalff,
saponifi3d,
shilman,
theinterned,
tmeasday and
z4o4z
as code owners
|
|
||
| storiesOf('Components|Tabs', module) | ||
| .addDecorator(s => <div style={{ height: 'calc(100vh - 20px)' }}>{s()}</div>) | ||
| .add('statefull - no initial', () => <TabsState panels={panels} />) |
|
@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! |
|
Why don't we whack an "in progress" label on it so others like me don't get confused |
|
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! |
We'd be forcing addon developers to adopt propTypes... Not everyone uses proptypes. |
|
@Hypnosphi thanks, I refactored the Tabs component, do you like it better this way? |
|
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 |
There was a problem hiding this comment.
Personally not a huge fan of these. Is there a reason?
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
Personal preference, add a conditional to return null instead of using a ternary.
There was a problem hiding this comment.
What would your solution look like?
An if statement above?
There was a problem hiding this comment.
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 | |||
| @@ -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", | |||
There was a problem hiding this comment.
When you remove emotion, what's left is abstract emptiness. Mah hart. Mah sole...
|
|
||
| if (!active) { | ||
| return null; | ||
| } | ||
| if (!backgrounds.length) return <Instructions />; |
There was a problem hiding this comment.
Newline between statements, and curly braces? Not sure what the rules here are.
There was a problem hiding this comment.
If eslint passes then it's OK
Issue: #2423 #3679
What I did
Also