giphy project by oscarGrandeG · Pull Request #75 · wizelineacademy/react-series-challenge

oscarGrandeG · 2018-07-09T04:57:36Z

No description provided.

add functionality to add favorites

parameter that supports language is added

modified initial state

timgivois · 2018-07-10T22:03:57Z

+// UI
+import UI from './Gifs.ui';
+
+class Search extends Component {


I'm not sure why are you using two components to map one container. You could just use one component and one container.

timgivois · 2018-07-10T22:05:58Z

+
+import UI from './Search.ui';
+
+class Search extends Component {


Same thing, why are you using two components and one container? I still believe that you could just use one :).

timgivois · 2018-07-10T22:06:13Z

+// UI
+import UI from './SearchResult.ui';
+
+class SearchResult extends Component {


Same comment

timgivois · 2018-07-10T22:06:42Z

+      const { add } = action;
+      const favoriteList = [...newState, add];
+
+      localStorage.setItem('favorites', JSON.stringify(favoriteList));


this are side effects and should be done by a sagas rather than a reducer.

timgivois · 2018-07-10T22:06:49Z

+        ...state.slice(i + 1)
+      ];
+
+      localStorage.setItem('favorites', JSON.stringify(deleteFavorite));


timgivois · 2018-07-10T22:07:00Z

+import gif from './getAllTrendingGifs';
+import searchGif from './searchGif';
+import favorites from './favorites';
+// import GifFilter from './GifFilter';


delete unused code

timgivois · 2018-07-10T22:07:20Z

+export function* watchGetAllTrendingGif() {
+    try {
+        const options = {
+            url: 'http://api.giphy.com/v1/gifs/trending?limit=25&offset=50&rating=g&api_key=8lCcODDfELugUIKA3mCMdGbp1UBdtx15&lang=es'


use a .env for api keys, for security

timgivois · 2018-07-10T22:07:45Z

+export function* searchGifs(res) {
+    try {
+        const options = {
+            url: `http://api.giphy.com/v1/gifs/search?q=${res.result}&api_key=8lCcODDfELugUIKA3mCMdGbp1UBdtx15&limit=5&lang=es`


same comment

timgivois · 2018-07-11T16:10:56Z

@@ -2,7 +2,5 @@ import React from 'react';
 import ReactDOM from 'react-dom';
 import './index.css';


use injectGlobal

timgivois · 2018-07-11T16:11:37Z

+import { Provider } from 'react-redux';
+import { BrowserRouter as Router, Route, Switch, Redirect } from 'react-router-dom';
+
 import './App.css';


use styled-components please

bgilm · 2018-07-11T21:00:30Z

-      </div>
+      <Provider store={store}>
+        <Router>
+          <Switch>


bgilm · 2018-07-11T21:00:43Z

+        <Router>
+          <Switch>
+            <Route exact path="/" component={Gifs} />
+            <Route exact path="/search/:q" component={SearchResult} />


bgilm · 2018-07-11T21:01:45Z

+            <Route exact path="/search/:q" component={SearchResult} />
+            <Route path="/favorites" component={Favorites} />
+            <Route path="*" component={NotFound} />
+            <Redirect to="not-found"/>


This redirect wont get called

bgilm · 2018-07-11T21:05:01Z

+      <div>
+        <h5>Pagina no encontrada.</h5>
+        <div>
+          <Link to="/">Regresar a inicio.</Link>


Robfz

Some general feedback, looks good so far. Also, consider adding new lines at the end of files.

Robfz · 2018-07-12T03:40:18Z

+export const DELETE_FAVORITES = 'DELETE_FAVORITES';
+export const FILTER_FAVORITES = 'FILTER_FAVORITES';
+
+function action(type, payload = {}) {


Really liked your approach to the payload definition, and the fact that you are giving a default value to it. I also like the way you are wrapping the payload inside an object 👍

Robfz · 2018-07-12T03:40:38Z

+export default connect(mapStateToProps, {
+  deleteFav,
+  getFavorites,
+  filterFavorites,


Robfz · 2018-07-12T03:41:42Z

+// actions
+import { add, getFavorites, deleteFav } from "./../../actions";
+
+class Gifs extends Component {


I think this could be a functional component.

Robfz · 2018-07-12T03:42:59Z

+import Search from './../Search/Search';
+import Menu from './../Sidebar/Sidebar';
+
+class SearchResult extends Component {


This component could also be a functional one.

Robfz · 2018-07-12T03:44:27Z

+const gif = (state = {}, action) => {
+  switch (action.type) {
+    case actions.FILTER_FAVORITES:
+      const newObj = { ...action }


Try to give some meaningful names to variables. This could be named newState.

Robfz · 2018-07-12T03:45:27Z

@@ -0,0 +1,22 @@
+import * as actions from "../actions";
+const favorite = {
+  add: [],


What is this?

Robfz · 2018-07-12T03:48:30Z

+  switch (action.type) {
+    case actions.SEARCH_RESULT:
+      return action.result;
+    case actions.SEARCH_EVENT:


You can write these cases like:

case actions.SEARCH_RESULT: case actions.SEARCH_EVENT: return action.result;

Robfz · 2018-07-12T04:03:04Z

+import * as actions from '../actions';
+
+export function* watchFavorites() {
+  const storage = JSON.parse(localStorage.getItem('favorites'));


Great! These side effects are perfectly encapsulated in Sagas.

Robfz · 2018-07-12T04:07:31Z

+        const resultSearch = yield call(ajax, options);
+        yield put(actions.searchResult(resultSearch));
+    } catch (error) {
+        yield put({ type: actions.GET_TRENDING_GIF_FAILED, error });


Use an action creator here.

OscarGG0408 and others added 30 commits July 7, 2018 21:55

new dependencies are added

4b7aeac

First version of redux saga is added

dcaa698

First version of the store is added

fcba507

First version of the action and reducer is added

a97fbee

new function is added to make requests to the API

955a7c5

first version of the view

82617ec

separate by modules the reducer and the saga

f5eb8b5

first implementation of the search engine

8fbe55a

fix: improvements in the definition of actions

fa28d32

fix: add redux to search engine

9c048ae

fix: functionality to dispatch search engine action

a250890

fix: correct error of call

03c004e

6a0b730

add functionality to add favorites

eec384b

parameter that supports language is added

c615bf1

modified initial state

routes are added

f685d24

the search component is extracted to an external component

ef30337

Gif catalog and search result component is created

317bde7

favorite template added

5963fcf

error correction when bookmarking

c7a0cc6

fix: favorite route added

47e1290

not found template added

9543916

sidebar added

13219ff

fix: joins menu in gif catalog and favorites

ff251e3

fix: add redirect

b43e3aa

fix: functionality to remove gifs

201e291

fix: menu and correction are added in a redux action

f78f420

fix: Actions are added to remove gifs

c167848

add filter functionality

4c8efc3

improvements are added to show favorites

e265269

oscarGrandeG added 3 commits July 10, 2018 09:18

fix: resolve error in add favorite action

f40aca4

code cleanup

d7139c0

fix: Favorites error is resolved

5ee6920

timgivois reviewed Jul 10, 2018

View reviewed changes

oscarGrandeG added 3 commits July 10, 2018 21:28

delete UI component

f4bbfca

migrate add favorites and delete favorites in redux saga

1b6dd3b

initialize npm run eject

cf175d9

timgivois reviewed Jul 11, 2018

View reviewed changes

bgilm reviewed Jul 11, 2018

View reviewed changes

Comment thread src/App.js

</div>

<Provider store={store}>

<Router>

<Switch>

bgilm Jul 11, 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.

👍

bgilm reviewed Jul 11, 2018

View reviewed changes

Robfz reviewed Jul 12, 2018

View reviewed changes

add unit test

e69f0f8


		import UI from './Search.ui';

		class Search extends Component {

		@@ -2,7 +2,5 @@ import React from 'react';
		import ReactDOM from 'react-dom';
		import './index.css';

Uh oh!

Conversation

oscarGrandeG commented Jul 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Robfz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants