-
Notifications
You must be signed in to change notification settings - Fork 52
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
Mini challenge 4 #61
base: master
Are you sure you want to change the base?
Mini challenge 4 #61
Conversation
- YouTubeProvider to handle context. - Accepting search parameter to useVideos.
- VideoDetail displaying title and description. - useRelatedVideos hook. - layout improved. - RelatedVideos in progres...
- handling error from YouTube API
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.
Hello Alfredo, I like what you have up to this point. One thing that is missing is some tests. I see a lot of 'should render{name} component correctly'
but you can test some events, the custom hooks, or the providers, I'd like improvements in the next delivery. I left some comments, just minor things. Great work 👍
import React from 'react'; | ||
import { ThemeProvider } from 'styled-components'; | ||
import { useYouTube } from '../YouTube/YouTubeProvider'; | ||
|
||
const theme = { | ||
dark: { | ||
primary: '#4396f3', | ||
secondary: '#4a4646', | ||
color: '#f7f7f7', | ||
}, | ||
}; | ||
|
||
function MyThemeProvider({ children }) { | ||
const { state } = useYouTube(); | ||
const { theme: stateTheme } = state; | ||
return <ThemeProvider theme={theme[stateTheme] || {}}>{children}</ThemeProvider>; | ||
} | ||
|
||
export default MyThemeProvider; |
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.
This theme provider looks good!
const url = ['https://www.googleapis.com/youtube/v3/search?']; | ||
url.push(`key=${process.env.REACT_APP_YOUTUBE_API_KEY}`); | ||
url.push(`&part=snippet&maxResults=10&type=video`); | ||
if (search) url.push(`&q=${search}`); |
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.
Took me a while to catch why this was an array, I think that concatenating strings by using the +
operator is more expressive, at first sight you know that 'asdf' + 'ewer' = 'asdfqwer'
and in this case you have an array of strings, you push items and at the end you do url.join('')
which I think is somewhat convoluted, at the end of the day it is just preference
const [videos, setVideos] = useState([]); | ||
const [isLoading, setLoading] = useState(false); | ||
const [error, setError] = useState(null); |
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.
Great, I like that you are handling the data, the loading state and the possible errors
return { videos, isLoading, error }; | ||
} | ||
|
||
export default useRelatedVideos; |
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.
Both of the hooks, the useReleatedVideos
and useVideos
, are well defined 👍
case 'search': | ||
return { | ||
...state, | ||
search: action.payload, | ||
}; | ||
case 'currentVideo': | ||
return { | ||
...state, | ||
currentVideo: action.payload, | ||
}; | ||
case 'closeCurrentVideo': | ||
return { | ||
...state, | ||
currentVideo: null, | ||
}; | ||
case 'switchTheme': | ||
return { | ||
...state, | ||
theme: state.theme === 'dark' ? 'default' : 'dark', | ||
}; |
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.
This actions seems really useful
theme: state.theme === 'dark' ? 'default' : 'dark', | ||
}; | ||
default: { | ||
return state; |
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'd throw an error here, it can prevent some unwanted stuff to go unnoticed, for example you can pass CloseCurrentVideo
and the state won't update but it will be hard to find that the bug is caused because the C
should be lower case like closeCurrentVideo
, I learned it the hard way 😆
Hi,
This PR contains both mini-challenges: 3 and 4.
Thanks a lot.