Skip to content

Challenge (Diego Plascencia)#72

Open
diego-d5000 wants to merge 25 commits into
wizelineacademy:masterfrom
diego-d5000:master
Open

Challenge (Diego Plascencia)#72
diego-d5000 wants to merge 25 commits into
wizelineacademy:masterfrom
diego-d5000:master

Conversation

@diego-d5000

@diego-d5000 diego-d5000 commented Jul 9, 2018

Copy link
Copy Markdown

PR for complete the 'React Series Challenge' for React Course at Wizeline Academy CDMX (Diego Plascencia)

@diego-d5000 diego-d5000 changed the title PR for Challenge (Diego Plascencia) Challenge (Diego Plascencia) Jul 9, 2018
…story deletion necessary), add api keys to env and add env example
Comment thread .env.example
@@ -0,0 +1 @@
REACT_APP_GIPHY_API_KEY=OWyGqcD3SWfggMMzdErRe9yHvasf2BBq No newline at end of file

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

you shouldn't save the api key in the .env.example

@diego-d5000 diego-d5000 Jul 10, 2018

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.

It's ok, it's just an example, It's a fake key haha

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

oh, ok haha nice.

const filteredGifs = this.getFilteredGifs(gifs);
return (
<Fragment>
<NavBar onSearch={(search) => this.setState({ favoritesSearch: search })} />

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

you can do this with redux I think

@@ -0,0 +1,88 @@
import { Row } from './Grid.styled';
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.

Nice I liked the styled components :)

@@ -0,0 +1,42 @@
import styled from 'styled-components';

const breakXs = '480px';

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/index.css
padding: 0;
font-family: sans-serif;
}
background-color: #fafafa;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

you can create a inject global

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.

Ok. I missed the option of use injectGlobal, but with your talk yesteday now I know how to solve this and use styled components instead of this css.

Very nice talk, thank you 😃 .

const { gifs } = this.props;
const filteredGifs = this.getFilteredGifs(gifs);
return (
<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.

👍

@Robfz Robfz left a comment

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.

Solid job.

Comment thread src/actions/giphyApi.js
const RECEIVE_NEXT_PAGE_TRENDING_GIFS = 'RECEIVE_NEXT_PAGE_TRENDING_GIFS';

const { fetchTrendingGifs, receiveTrendingGifs, receiveNextPageTrendingGifs } = createActions({
[FETCH_TRENDING_GIFS]: (search = '', page = 0) => ({ search, page }),

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.

Beautiful stuff 👌

}

getFilteredGifs = (gifs) => {
const { favoritesSearch } = this.state;

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.

This logic could be moved to the Redux store.

Comment thread src/components/Home.js
}

handleLoadMoreData = () => {
this.setState((prevState, props) => {

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.

Functional state + setState callback, nice 👍

Comment thread src/components/Home.js
constructor(props) {
super(props);
this.state = {
search: '',

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.

This search term could be reused with the other view and stored in the Redux store.

Comment thread src/components/NavBar.js
class NavBar extends Component {
constructor(props) {
super(props);
this.state = { search: '' }

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.

Same thing about the search.

Comment thread src/reducers/favorites.js
@@ -0,0 +1,23 @@
import { handleActions } from 'redux-actions';

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/sagas/giphyApi.js
const PAGE_SIZE = 20;

// taken from:
// https://stackoverflow.com/questions/6566456/how-to-serialize-an-object-into-a-list-of-url-query-parameters

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.

Giving other devs credit is a great practice, keep it up!

Comment thread src/store.js
import loggerMiddleware from './middleware/logger';
import rootReducer from './reducers';
import rootSaga from './sagas';
import persistState from 'redux-localstorage';

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.

Cool!

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