-
Notifications
You must be signed in to change notification settings - Fork 12
mini challenge 3: hooks #6
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
base: master
Are you sure you want to change the base?
Conversation
dianasaurio
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had problems with your netlify app, ping me if you fix it
.env
Outdated
| REACT_APP_YOUTUBE_PLAYLIST_ITEMS_API = "https://youtube.googleapis.com/youtube/v3/playlistItems" | ||
| REACT_APP_YOUTUBE_CHANNELS_LIST_API = "https://www.googleapis.com/youtube/v3/channels" | ||
| REACT_APP_YOUTUBE_SEARCH = "https://content-youtube.googleapis.com/youtube/v3/search" | ||
| REACT_APP_YOUTUBE_API_KEY="AIzaSyAx6keB9STIcO-bkIKuy-ubU6CZO9pnwXQ" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest you don't commit sensitive data, you can add a .env.example with placeholder values and add the .env extension to the .gitignore
| @@ -0,0 +1,3 @@ | |||
| window.env = { | |||
| "YOUTUBE_API_KEY": "AIzaSyAx6keB9STIcO-bkIKuy-ubU6CZO9pnwXQ" | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need this file if you have a .env?
| return ( | ||
| <ButtonContainer> | ||
| <span> | ||
| <img className="image" alt="User Avatar" src="https://media.glassdoor.com/sqll/868055/wizeline-squarelogo-1473976610815.png" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will look cleaner if you use a variable for the image URL and pass it to the src.
| @@ -0,0 +1,23 @@ | |||
| import React from 'react'; | |||
| import Button from './Button.component'; | |||
| import renderer from 'react-test-renderer'; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest you use react-testing-library since it's the recommended approach
src/components/LeftNave.jsx
Outdated
|
|
||
| const LeftNav = ({ open }) => { | ||
| return ( | ||
| <Ul open={open}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try to give your components descriptive names, using the same names as HTML tags can cause confusion when reading code.
|
|
||
| import { Box, BoxImage, BoxInfo, BoxTitle, BoxText } from './styled'; | ||
|
|
||
| const NoImage = require('./noimage.jpg'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's best to use import instead of require for react apps
| @@ -0,0 +1,36 @@ | |||
| import { useEffect, useState } from "react"; | |||
| import { useParams } from 'react-router-dom'; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's best to use react-router to handle URL params:
https://reactrouter.com/web/example/url-params
| const add = "add"; | ||
| const remove = "remove"; | ||
| let { id } = useParams(); | ||
| const getArray = JSON.parse(localStorage.getItem('favorites')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
localStorage is not a recommended state management solution for react. I suggest you use context instead.
https://medium.com/@harshdigwani/redux-vs-context-api-vs-local-storage-bced5118c531
src/components/useFetch.js
Outdated
| import { useEffect, useState } from "react"; | ||
|
|
||
| export const useFetch = (url) => { | ||
| const [state, setState] = useState({items:null, isLoaded:true, hasErrors: false}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest you separate each state property into its own state variables because when you are calling setState you are overwriting the whole state and it's more error-prone.
| @@ -0,0 +1,7 @@ | |||
| const sum = require('./routine'); | |||
|
|
|||
| test('adds 1 + 2 to equal 3', () => { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this file is not meant to be committed (:
|
Also, I don't see any coverage report, let me know if you have it |

No description provided.