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
Teacher UI in progress redesign #13566
Conversation
Bjvanminnen
commented
Mar 2, 2017
rightCol: { | ||
display: 'table-cell', | ||
verticalAlign: 'top', | ||
// TODO - fixed width will likely get us into trouble for other languages |
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.
think about this some more brent
@@ -0,0 +1,74 @@ | |||
/** |
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.
Potentially contentious decision here and with HiddenStageToggle to name a component identically to the component it is replacing (the difference being where they live).
apps/src/redux.js
Outdated
@@ -61,19 +61,32 @@ if (IN_UNIT_TEST) { | |||
}; | |||
} | |||
|
|||
if (IN_STORYBOOK) { | |||
// Storybooks need the ability to create multiple distinct stores instead of | |||
// using a singletone |
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.
Singletone 🎶 or singleton?
*/ | ||
function createStoreWithReducers() { | ||
return createStore(Object.keys(globalReducers).length > 0 ? | ||
redux.combineReducers(globalReducers) : s => s); |
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.
Weird... how is line 84 covered but line 85 is not?
@@ -122,6 +122,7 @@ var storybookConfig = _.extend({}, baseConfig, { | |||
new webpack.ProvidePlugin({React: 'react'}), | |||
new webpack.DefinePlugin({ | |||
IN_UNIT_TEST: JSON.stringify(false), | |||
IN_STORYBOOK: JSON.stringify(true), |
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.
I'm surprised storybook doesn't already provide this somehow, but I looked around and it doesn't seem to. 👍
}, | ||
}; | ||
|
||
const HiddenStageToggle = React.createClass({ |
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.
Might not be worth it yet, but I bet extracting a generic toggle component in this new style will be useful down the road.
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.
Looking good. It seems like some of your new components' render methods aren't called in unit tests, might want to see if there's anything else you want to cover.