Skip to content

REACT SERIES CHALLENGE - Fist Delivery#60

Open
giovas94 wants to merge 32 commits into
wizelineacademy:masterfrom
giovas94:master
Open

REACT SERIES CHALLENGE - Fist Delivery#60
giovas94 wants to merge 32 commits into
wizelineacademy:masterfrom
giovas94:master

Conversation

@giovas94

@giovas94 giovas94 commented Jul 7, 2018

Copy link
Copy Markdown

Fist commit for challenge

@giovas94 giovas94 changed the title Initial commit REACT SERIES CHALLENGE Jul 8, 2018

if (!filtered) {
return (
<div>{favoritesArr.map(key => <Item key={key} item={items[key]} />)}</div>

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Remember you can avoid unnecessary wrapping elements in React16

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Modified, thanks!

Comment thread src/Routes.js
import Favorites from "./Pages/Favorites";

const Routes = () => (
<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/Routes.js
<Switch>
<Route path="/favorites" component={Favorites} />
<Route path="/" component={Home} />
</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.

It is a nice to have to catch all unknown routes and redirect them either to a not found route or to the index with a message for the user.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Modified, thanks!

Comment thread src/api/giphy.js Outdated
@@ -0,0 +1,4 @@
const HOST = "https://api.giphy.com/";
const API_KEY = "Zx5PYfLyHZFhzV4bFwtohSoObgCckVUU";

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Use .env variables to save your api keys and do not expose them versioning them. You can provide an .env.example file expressing its usage.

https://www.npmjs.com/package/dotenv

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Modified, thanks!

@giovas94 giovas94 changed the title REACT SERIES CHALLENGE REACT SERIES CHALLENGE - Fist Delivery Jul 10, 2018
`;

NavLinkStyled.defaultProps = {
activeClassName: "active"

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/Routes.js
<Switch>
<Route exact path="/" component={Home} />
<Route path="/favorites" component={Favorites} />
<Redirect from="/favorite" to="/favorites" />

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/Routes.js
import NoMatch from "./Pages/NoMatch";

const Routes = () => (
<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/Routes.js
import Favorites from "./Pages/Favorites";
import NoMatch from "./Pages/NoMatch";

const Routes = () => (

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

In this specific exercise you may not see the value but you are declaring the routes in a semi static way on a separate file. Remember that one of the key takeaways of react router 4 is integrating the routes through the component views dynamically, as your application grows and become more complex maintaining this static definition of routes may not be the best approach

Comment thread src/api/giphy.js
@@ -0,0 +1,4 @@
const HOST = process.env.REACT_APP_GIPHY_HOST;
const API_KEY = process.env.REACT_APP_GIPHY_API_KEY;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👍

<NavbarStyled>
<LogoContainer />
<NavStyled>
<NavLinkStyled exact to="/">

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
<Provider store={store}>
<PersistGate loading={null} persistor={persistor}>
<Router>
<React.Fragment>

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 README.md
6. Styling
- Use styled components

1. :white_check_mark: Use Giphy API

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice!


export default connect(
mapStateToProps,
null

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If you are not using the mapDispatchToProps, you can just omit the parameter, no need to use null 👍

const mapDispatchToProps = dispatch => {
return {
onToggleFavorite: item =>
dispatch({ type: "FAVORITE_TOGGLE", payload: { item } })

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You can use bindActionCreators or the object notation to avoid having to wrap the dispatch manually around the creators.

} = this.props;

if (
keyCode === 13 ||

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

As a tip, always use const for these kind of cases:

const ENTER_KEY_CODE = 13;

Comment thread src/reducers/index.js
const rootReducer = combineReducers({
trendGifs,
searchGifs,
favorites: persistReducer(favoritesPersistConfig, favorites),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍 nice

Comment thread src/actions/search.js
const SEARCH_INPUT = "SEARCH_INPUT";
const SEARCH_FAVORITE = "SEARCH_FAVORITE";

const setSearchInput = (query = "") => ({ type: SEARCH_INPUT, query });

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Remember that the related data of an action should be contained in a payload attribute.

Comment thread src/sagas/searchGifs.js
const { data } = response.data;

// dispatch a success action to the store with the new data
yield put({ type: "API_CALL_SEARCH_SUCCESS", data });

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Always try to use action creators.

Comment thread src/actions/trendGifs.js
const API_CALL_SUCCESS = "API_CALL_SUCCESS";
const API_CALL_FAILURE = "API_CALL_FAILURE";

export default {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Missing creators.

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.

4 participants