Create component for Recipe card, Add storybook support #41
Conversation
__tests__/ReactElementType.js
Outdated
| }; | ||
|
|
||
| describe('ReactElementType', () => { | ||
| it('map contains alltest types', () => { |
src/components/Like/Like.scss
Outdated
| font-size: 1em; | ||
| color: #7c8a94; | ||
|
|
||
| &:focus {outline:0;} |
package.json
Outdated
| @@ -7,6 +7,8 @@ | |||
| "start": "webpack-dev-server --open", | |||
| "build": "webpack", | |||
| "test": "jest", | |||
| "clean": "rm -rf node_modules", | |||
| "reinstall": "npm run clean && npm install", | |||
There was a problem hiding this comment.
should also probably add npm cache clean here so we dont have any persisting modules from cache
__tests__/ReactElementType.js
Outdated
| @@ -0,0 +1,13 @@ | |||
| const ReactElementType = { | |||
There was a problem hiding this comment.
We should probably have .test in the file name to be consistent.
src/components/Chip/Chip.stories.jsx
Outdated
| .add('Calcium', () => ( | ||
| <Chip name="calcium" /> | ||
| )) | ||
| .add('Vitamin B2', () => ( |
There was a problem hiding this comment.
Unnecessary stories here, one should be enough.
Any time we're just showing different text for a component, we should reconsider the use of storybook. It a good indication that we're using storybook incorrectly. It's meant to show different states and not different text.
There was a problem hiding this comment.
Talked offline and agreed with the point made.
| import Yogurt from 'images/ingredients/yogurt.png'; | ||
|
|
||
| storiesOf('IngredientCard', module) | ||
| .add('Milk', () => ( |
There was a problem hiding this comment.
Again, will we do this for every ingredient? This is not what storybook is meant to do... We're literally changing just text.
If you want to show the theme differences, I think it would make more sense for us to use One ingredient with all the different themes.
| @keyframes heart-burst { | ||
| from {background-position:left;} | ||
| to { background-position:right;} | ||
| } |
There was a problem hiding this comment.
Incorrect line ending
| <div | ||
| className="recipe-card-image" | ||
| alt={title} | ||
| style={{ backgroundImage: `linear-gradient(to bottom, rgba(255,255,255,0) 20%, rgba(245, 242, 242, 1)), url(${image})` }} |
There was a problem hiding this comment.
What are your thoughts on using StyledComponents to do this?
There was a problem hiding this comment.
Maybe not a great idea to use a lib for such a small thing but might be cleaner...
There was a problem hiding this comment.
@sukritchhabra Because I need to edit a style and not an html element attribute. The style needed here is background-image. Any better suggestions IYO?
There was a problem hiding this comment.
What lib are you talking about? font-awesome?
There was a problem hiding this comment.
There was a problem hiding this comment.
Yeah its a neat library that would make the code a bit cleaner but I'm not sure about adding a lib for this too..
@sukritchhabra how about we keep this in the back-pockets and file a backlog task. And if we have a lot of style injections like we are doing above then we can utilize the library for a cleaner way?
| import RecipeCard from 'components/RecipeCard/RecipeCard'; | ||
|
|
||
| storiesOf('RecipeCard', module) | ||
| .add('Lemon Grass Chicken', () => ( |
There was a problem hiding this comment.
Again, all these stories are unnecessary, they're literally just showing different text...
src/stories/index.js
Outdated
| @@ -0,0 +1,7 @@ | |||
| import 'components/Chip/Chip.stories'; | |||
| import 'components/CardContainer/CardContainer.stories'; | |||
There was a problem hiding this comment.
Let's move the stories out of src
.storybook/config.js
Outdated
| @@ -1,7 +1,7 @@ | |||
| import { configure } from '@storybook/react'; | |||
|
|
|||
| function loadStories() { | |||
| require('../src/stories'); | |||
| require('../src/storybook'); | |||
There was a problem hiding this comment.
Sorry, I didn't mean we should just scope the folders. I mean't to decouple storybook from src completely. So the stories should live in the stories folder as a sibling to src.
- package.json
- src/
- stories/
- .storybook/
This repo is a good example. https://github.com/airbnb/react-dates
There was a problem hiding this comment.
That might not be the move. I have seen plenty of repos that keep the stories in the src. Following our existing convention of __test__ being in src which utilizes the components, stories should also be in src as they too utilize components.
Further-more:
Here is what the official doc says:
"There’s no hard and fast rule for this. But, keeping stories close to your components is a good idea.
For example, let’s say your UI components live in a directory called: src/components. Then you can write stories inside the src/stories directory.
This is just one way to do that. You can always edit your storybook config file and ask it to load stories from anywhere you want."
There was a problem hiding this comment.
Our tests aren't in src. And I personally prefer things to be decoupled as much as possible. In fact, since our tests are decoupled outside of src, it would make more sense to decouple the stories too.
resolves #16