Skip to content

Daniel Garcia submission#71

Open
pel-daniel wants to merge 47 commits into
wizelineacademy:masterfrom
pel-daniel:master
Open

Daniel Garcia submission#71
pel-daniel wants to merge 47 commits into
wizelineacademy:masterfrom
pel-daniel:master

Conversation

@pel-daniel

Copy link
Copy Markdown

No description provided.

@@ -0,0 +1,18 @@
import { injectGlobal } 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.

super cool 💯

Comment thread src/styles/Global.style.js Outdated
font-family: sans-serif;
}

.AppContainer {

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 still using classNames?

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.

I use .AppContainer in 2 places, the header and the entire app. Is there a better way to do it?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Yes, you shouldn't use classnames, you can create an AppContainer with Styled componets that implements this. Check https://www.styled-components.com/docs/basics#getting-started

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.

Makes sense! Thanks!

I also have the class mt that is just a margin top. Do you an example of how to handle this without css classes?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

haven't used mt before, so I believe that it has a css that you can copy. (Sort of thing I did with BootStrap buttons)

Comment thread src/sagas/index.js
try {
const response = yield call(
fetchGifs,
`${api_base_url}/trending?api_key=${process.env.REACT_APP_GIPHY_API_KEY}&limit=15`

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/components/AppContainer.js Outdated
const { gifs, favorites } = this.props;

return (
<div className="AppContainer mt">

@timgivois timgivois 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.

don't use classnames, avoid global css

@pel-daniel pel-daniel changed the title WIP First week delivery Daniel Garcia submission Jul 11, 2018

@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.

👍

@@ -0,0 +1,14 @@
import { createAction } 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.

👍

Comment thread src/actions/gifActions.js

const failedSearchGifs = createAction(SEARCH_GIFS_FAILED);
const receivedSearchGifs = (payload = {}) => ({ type: SEARCH_GIFS_RECEIVED, payload });
const searchGifs = (payload = {}) => ({ type: SEARCH_GIFS_GET, payload });

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.

Why did you mix the usage of createAction and a manual declaration?

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.

Comment thread src/sagas/index.js
try {
const response = yield call(
fetchGifs,
`${api_base_url}/search?q=${payload}&api_key=${process.env.REACT_APP_GIPHY_API_KEY}&limit=15`

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.

Good use of env variables 👍

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.

3 participants