Skip to content

my challenge pr#65

Open
Lvcios wants to merge 17 commits into
wizelineacademy:masterfrom
Lvcios:master
Open

my challenge pr#65
Lvcios wants to merge 17 commits into
wizelineacademy:masterfrom
Lvcios:master

Conversation

@Lvcios

@Lvcios Lvcios commented Jul 8, 2018

Copy link
Copy Markdown

No description provided.

Comment thread src/middleware/gifFetcher.js Outdated
const API_URL = 'https://api.giphy.com/v1/gifs/';

const fetchTrending = (dispatch) => {
axios.get(`${API_URL}trending?api_key=SA3WrTOcAq5bNF9hsWC4Z8j0O1UkOMuB&limit=10&rating=G`)

@timgivois timgivois Jul 8, 2018

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'll advise that you put your api_key in .env and provide a .env.example

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Comment thread src/components/Favs/index.js Outdated
}
}

const mapStateToProps = (state) => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'll advise that you divide components and containers in different files. It's easier to read and check.

Comment thread src/components/Home/index.js Outdated

}

removeFav = (event) =>{

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

this should be in the container, not in the component.

Comment thread src/components/Home/index.js Outdated
this.props.trendingGifs.query = value
}

addFav = (event) =>{

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

this should be in the container, not in the component.

@timgivois

Copy link
Copy Markdown

As general feedback, try to create (almost) stateless components and migrate all the logic to containers.

Comment thread src/App.js
<li><NavLink to="/favs">Your favs</NavLink></li>
<li><NavLink to="/">Trending</NavLink></li>
</ul>
<Switch>

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👍

Comment thread src/App.js Outdated
<div className="App-intro">
<h1>React Challenge</h1>
<ul>
<li><NavLink to="/favs">Your favs</NavLink></li>

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👍

Lvcios added 2 commits July 11, 2018 13:47
first style components
separate components
Comment thread src/App.css Outdated
@@ -23,6 +23,13 @@
}

@keyframes App-logo-spin {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

please use styled components for the keyframes. Delete all the css and use the styled components

}

return (
<div className="home">

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

this could be a styled component


class Page404 extends Component{
render(){
return <div className='not-found'>

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

a styled component ttoo

Comment thread src/App.styles.js
@@ -0,0 +1,60 @@
import styled from "styled-components";

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I will advise that you create a .style.js file per component. Not a global one

Lvcios added 2 commits July 11, 2018 17:56
remove global app.css
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants