Pull Request - Francisco Rafael Arce García #58

fragmnt02 · 2018-07-06T21:55:10Z

No description provided.

timgivois · 2018-07-08T01:13:55Z

.env

@@ -0,0 +1,2 @@
+REACT_APP_API_KEY=iSAbLNTnHlhG33VP27yEQDITzhxAZ1iZ


I recommend that you add this file to your .gitignore as this are api keys

Done! Thnks

Done. Thnks.

You pushed it? I can still see your .env config

charliesbot · 2018-07-09T02:09:52Z

src/App.js

-          To get started, edit <code>src/App.js</code> and save to reload.
-        </p>
-      </div>
+        <React.Fragment>


fragments, well done!

charliesbot · 2018-07-09T02:17:41Z

src/Containers/SearchPage/saga.js

+
+function* doQuerySearch({ param = {} }) {
+  var q;
+  param.q===""||param.q===undefined||param.q===null? q="pato":q=param.q;


kinda

const q = !para.q ? "pato" : param.q;

if someone for some reason get in the search view without looking for something the "pato" search happen.

timgivois · 2018-07-10T21:55:33Z

src/Containers/SearchPage/index.js

+  componentDidMount() {
+    const { querySearch } = this.props;
+    querySearch({q:this.props.location.state.query});
+    console.log();


I advise you delete console.log

timgivois · 2018-07-10T21:55:40Z

src/Containers/SearchPage/index.js

+    const { querySearch } = this.props;
+    querySearch({q:this.props.location.state.query});
+    console.log();
+    //


delete this comment

timgivois · 2018-07-10T21:57:23Z

src/components/Gif.js

+        super(props);
+        this.state = {
+            buttonText: "Fav",
+            fav: false


I think that you are replicating Redux store with the state. redux store should be the only source of true. Plus, If you can avoid states in your components would be better. (States only should be used for little things).

timgivois · 2018-07-10T21:57:48Z

src/components/Gif.js

+        return ( 
+            < React.Fragment >
+            <Wraper onClick = {this.onClick}>
+                <ImgStyled src={require("./"+this.state.buttonText+".png")}   />


Why you have 2 img?

One is a heart button and the other one is the gif

timgivois · 2018-07-10T21:58:16Z

src/components/Navbar.js

+class Navbar extends Component {
+  constructor(props) {
+    super(props);
+    this.state = {


Why are you using a state here if you don't update it

I update it when I write in my input

yes, you are right :)

timgivois · 2018-07-10T21:58:31Z

src/components/Navbar.style.js

+import styled from 'styled-components'
+
+export const UlStyled=styled.ul`
+{


you don't need brackets

timgivois · 2018-07-11T21:48:36Z

src/test/components/__snapshots__/Gif.style.test.js.snap

+
+exports[`Styled gif works 1`] = `
+<div
+  className="sc-bdVaJa jbzoYE"


use jest-styled-components for the snapshot.

timgivois · 2018-07-11T21:49:41Z

src/test/components/Navbar.style.test.js

@@ -0,0 +1,9 @@
+import React from 'react'
+import renderer from 'react-test-renderer'


timgivois · 2018-07-11T22:11:50Z

src/components/Navbar.style.js

+  background-color: #333;
+`
+export const LiStyled=styled.li`
+{


you don't need the brackets again

bgilm · 2018-07-11T22:54:39Z

src/router/Routes.js

+import Fav from "../Containers/FavPage";
+import Search from "../Containers/SearchPage";
+
+const Routes = () => {


Try to change into dynamic routing mindset, in this case every route is at the same index but defining your routes on a separate file before anything renders is reinforcing static practices.

As your application grow larger it might not make a lot of sense to isolate routes this way.

:D I will work on that. Thanks for your feedback

bgilm · 2018-07-11T22:54:57Z

src/router/Routes.js

+const Routes = () => {
+  return (
+    <React.Fragment>
+      <Switch>


bgilm · 2018-07-11T22:56:06Z

src/components/Navbar.js

+      <React.Fragment>
+        <UlStyled>
+          <LiStyled>
+            <NavLink to="/">Home</NavLink>


bgilm · 2018-07-11T22:56:15Z

src/components/Navbar.js

+            <NavLink to="/">Home</NavLink>
+          </LiStyled>
+          <LiStyled>
+          <NavLink to="/fav">Favs</NavLink>


Robfz · 2018-07-12T18:34:08Z

src/Containers/FavPage/action.js

+export function addFav(params) {
+  return {
+    type: ADD_FAV,
+    params


The related data of an action should be contained in a payload attribute.

ok. I'll consider it next time.

Robfz · 2018-07-12T18:36:39Z

src/components/Gif.js

+    }
+
+    onClick() {
+        if (this.props.gif.length > 0) {


You can avoid this state management leveraging the fav logic to the store.

I will work on that.

Robfz · 2018-07-12T18:37:34Z

src/configureStore.js

+    )
+  )
+
+  store.runSaga = sagaMiddleware.run


It is a bad practice to 'enhance' the store this way. Calling runSaga on the saga middleware is a better approach.

Ok. Thanks for your feedback.

fragmnt02 added 4 commits July 5, 2018 12:35

iniciando proyecto, instalando router y creando componentes

b7c75d4

agregando search

24e3a9b

agregando logica search

8f263e8

modificiacaion

c74cd07

timgivois reviewed Jul 8, 2018

View reviewed changes

fragmnt02 added 4 commits July 8, 2018 12:19

redux with errors

156cd04

trending and search with redux and sagas

6057123

Search and home works properly

7ead9e1

Adding env to .gitignore

54dc0e6

charliesbot reviewed Jul 9, 2018

View reviewed changes

fragmnt02 added 5 commits July 8, 2018 23:08

logica terminada

696fecd

fav working correctly

28628ee

remove fav

edaee3c

Details

69f40eb

deleting unused libraries

841e975

fragmnt02 changed the title ~~Lógica casi lista, falta implementación de Redux~~ Pull Request - Francisco Rafael Arce García Jul 9, 2018

fragmnt02 added 2 commits July 10, 2018 11:00

gif button change fixed

efcace5

gifs heart button working

9d7731d

timgivois reviewed Jul 10, 2018

View reviewed changes

src/components/Navbar.style.js Outdated

import styled from 'styled-components'

export const UlStyled=styled.ul`

{

Copy link

timgivois Jul 10, 2018

Choose a reason for hiding this comment

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

you don't need brackets

fragmnt02 added 5 commits July 10, 2018 17:18

feedback corrections

20bbf29

remove state from fav page

004dd63

favpage error fixed

f78658d

some tests

1ab462d

fix some errors

fbfe216

only working test

50ecfa1

timgivois reviewed Jul 11, 2018

View reviewed changes

src/test/components/Navbar.style.test.js

@@ -0,0 +1,9 @@

import React from 'react'

import renderer from 'react-test-renderer'

Copy link

timgivois Jul 11, 2018

Choose a reason for hiding this comment

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

use enzyme

timgivois reviewed Jul 11, 2018

View reviewed changes

bgilm reviewed Jul 11, 2018

View reviewed changes

src/router/Routes.js

const Routes = () => {

return (

<React.Fragment>

<Switch>

Copy link

bgilm Jul 11, 2018

Choose a reason for hiding this comment

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

👍

fragmnt02 reacted with heart emoji

bgilm reviewed Jul 11, 2018

View reviewed changes

src/components/Navbar.js

<React.Fragment>

<UlStyled>

<LiStyled>

<NavLink to="/">Home</NavLink>

Copy link

bgilm Jul 11, 2018

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

fragmnt02 added 7 commits July 11, 2018 22:07

some changes

70e030c

gif test

b38b715

fav reducer test fixed

84badf9

saga test

5590345

sagas tests

43499c4

sagas

fbdfbc7

unused state

98211ac

Robfz reviewed Jul 12, 2018

View reviewed changes

fix

ccf7d6d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pull Request - Francisco Rafael Arce García #58

Pull Request - Francisco Rafael Arce García #58

fragmnt02 commented Jul 6, 2018

timgivois Jul 8, 2018

fragmnt02 Jul 9, 2018

fragmnt02 Jul 9, 2018

timgivois Jul 10, 2018

charliesbot Jul 9, 2018

charliesbot Jul 9, 2018 •

edited

Loading

fragmnt02 Jul 9, 2018

timgivois Jul 10, 2018

fragmnt02 Jul 10, 2018

timgivois Jul 10, 2018

fragmnt02 Jul 10, 2018

timgivois Jul 10, 2018

timgivois Jul 10, 2018

fragmnt02 Jul 10, 2018

timgivois Jul 10, 2018

fragmnt02 Jul 10, 2018

timgivois Jul 10, 2018

fragmnt02 Jul 10, 2018

timgivois Jul 10, 2018

timgivois Jul 11, 2018

timgivois Jul 11, 2018

timgivois Jul 11, 2018

fragmnt02 Jul 12, 2018

bgilm Jul 11, 2018

fragmnt02 Jul 12, 2018

bgilm Jul 11, 2018

bgilm Jul 11, 2018

bgilm Jul 11, 2018

Robfz Jul 12, 2018

fragmnt02 Jul 12, 2018

Robfz Jul 12, 2018

fragmnt02 Jul 12, 2018

Robfz Jul 12, 2018

fragmnt02 Jul 12, 2018

		@@ -0,0 +1,2 @@
		REACT_APP_API_KEY=iSAbLNTnHlhG33VP27yEQDITzhxAZ1iZ

		@@ -0,0 +1,9 @@
		import React from 'react'
		import renderer from 'react-test-renderer'

Pull Request - Francisco Rafael Arce García #58

Are you sure you want to change the base?

Pull Request - Francisco Rafael Arce García #58

Conversation

fragmnt02 commented Jul 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charliesbot Jul 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charliesbot Jul 9, 2018 •

edited

Loading