Skip to content
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

React Series Challenge - Daniel #66

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

damontes
Copy link

@damontes damontes commented Jul 9, 2018

No description provided.

const Navbar = ({ location }) => (
<div>
<div>
<NavLink to="/">Home</NavLink>
Copy link

Choose a reason for hiding this comment

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

👍

.get("http://api.giphy.com/v1/gifs/search", {
params: {
q: query,
api_key: "NYBl3G1fuM3PcJWfeAv0wSS6fuHAJhIK"
Copy link

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

@@ -0,0 +1,35 @@
import { injectGlobal } from "styled-components";

Choose a reason for hiding this comment

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

nice

right: 10px;
top: 15px;
background-color: transparent;
color: ${props => props.favorite ? "#00a2ff" : "#ccc" };

Choose a reason for hiding this comment

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

👍

package.json Outdated
},
"jest": {
"collectCoverageFrom": [
"**/src/**/*.{js,jsx}",
"!**/src/registerServiceWorker.{js,jsx}"
"!**/src/registerServiceWorker.{js,jsx}",
"!**/node_modelus",

Choose a reason for hiding this comment

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

is this a typo?

import { Container } from "./App.style";
import NotFound from "./pages/NotFound";

const Routes = () => {
Copy link

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

const ADD_FAVORITE_GIF = "ADD_FAVORITE_GIF";
const GET_FAVORITE_SEARCH_GIFS = "GET_FAVORITE_SEARCH_GIFS";

export default {
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing action creators.


const mapDispatchToProps = dispatch => {
return {
addFavoriteGif: gif => dispatch({ type: "ADD_FAVORITE_GIF", payload: gif })
Copy link
Contributor

Choose a reason for hiding this comment

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

With creators you could have done something like:

const mapDispatchToProps = dispatch => bindActionCreators({
  addFavoriteGif: creators.addFavoriteGif,
}, dispatch);

or

const mapDispatchToProps = { addFavoriteGif: creators.addFavoriteGif };


export default connect(
mapStateToProps,
null
Copy link
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 mapDispatchToProps, you can omit the second parameter of connect;

const favoriteReducer = (state = initialState, action) => {
const { type, payload } = action;
switch (type) {
case favoriteGifsActions.types.ADD_FAVORITE_GIF: {
Copy link
Contributor

Choose a reason for hiding this comment

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

This reducer should have separate ADD_FAVORITE_GIF and REMOVE_FAVORITE_GIF.

import { createStore, applyMiddleware, compose } from "redux";
import createSagaMiddleware from "redux-saga";
import reducers from "../reducers";
import { persistStore, persistReducer } from "redux-persist";
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

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