1st PR#57
Conversation
| gifs: [] | ||
| } | ||
|
|
||
| componentDidMount() { |
There was a problem hiding this comment.
good first start, I recommend that you start migrating to redux and add the api keys to a .env file.
… prepare the code for the test with Jest
|
|
||
|
|
||
| /* Styles */ | ||
| .container { |
There was a problem hiding this comment.
you shouldn't use css, everything should be migrated to styled components
There was a problem hiding this comment.
Yep, I know Tim. I'm working right now to move the styles.
Thank you!!
| <div className=""> | ||
| <p>My Favorites</p> | ||
|
|
||
| <div className="container"> |
There was a problem hiding this comment.
you don't need classNames with styled components
| height: 80px; | ||
| } | ||
|
|
||
| .App-header { |
| <div> | ||
| <p>My Favorites</p> | ||
|
|
||
| <div className="container"> |
There was a problem hiding this comment.
don't use classNames use styled components
|
Thank you, Tim. I just removed that App.css file |
| import React from 'react'; | ||
| import ReactDOM from 'react-dom'; | ||
| import { Provider } from 'react-redux'; | ||
| import './index.css'; |
|
|
||
| function* doQuerySearch({ param = {} }) { | ||
| var q; | ||
| param.q===""||param.q===undefined||param.q===null? q="pato":q=param.q; |
There was a problem hiding this comment.
Avoid having hardcoded parameters e.g. "pato"
| export function queryTrending(param) { | ||
| return { | ||
| type: QUERY_TRENDING, | ||
| param |
There was a problem hiding this comment.
The related data of an action should be contained in a payload attribute.
| } | ||
|
|
||
| onClick() { | ||
| if (this.props.gif.length > 0) { |
There was a problem hiding this comment.
You can avoid this state management leveraging the fav logic to the store.
| export default function* rootSaga() { | ||
| yield all([ | ||
| fork(homePageWatcher), | ||
| fork(searchPageWatcher) |
| ) | ||
| ) | ||
|
|
||
| store.runSaga = sagaMiddleware.run |
There was a problem hiding this comment.
It is a bad practice to 'enhance' the store this way. Calling runSaga on the saga middleware is a better approach.
| render() { | ||
|
|
||
| return ( | ||
| <React.Fragment> |
4 participants
Hi, this is my 1st PR. I know I'll need to use redux-saga in order to make the API calls. I'll work on that later.