feat: react certification deliverable estefi#2
feat: react certification deliverable estefi#2estefijacobo wants to merge 16 commits intowizeline:masterfrom
Conversation
| case 'SET_CURRENT_VIDEO': | ||
| return { ...state, currentVideo: action.payload }; | ||
| case 'ADD_FAVORITE_VIDEO': { | ||
| const videoExists = () => { |
There was a problem hiding this comment.
You don't need to declare these functions inside the case, you can extract them and leave the case cleaner.
| favoriteVideos: state.favoriteVideos, | ||
| }; | ||
| } | ||
| localStorage.setItem( |
There was a problem hiding this comment.
I think that is better if inside the component that is updating the state you update the local storage, It was a convention in Redux to use only pure functions inside the reducers and I think it a good convention to follow with React as well.
dianaatwizeline
left a comment
dianaatwizeline
left a comment
There was a problem hiding this comment.
Most of the code looks good except for the test cases which are missing and some of the ones you have are failing.
The deployed application on Netlify needs some love, as I'm able to do everything I expect except for watching the video while on Video Detail view and there is a bug with the favorites in that sometimes adding a new favorite removes the previous favorites.
At this point I think it is more a thing with bugfixing and implementing more tests than React proficiency.
| function VideoCard({ videoDescription, videoTitle, videoThumbnail, videoId }) { | ||
| const { dispatch } = useVideoContext(); | ||
|
|
||
| const handleVideoClick = () => { |
There was a problem hiding this comment.
would be better if these functions lived inside useVideoContext and took the payload as arguments so you don't need to worry about dispatching from here and you have the wrapper ready if you want to use these functions elsewhere
| import { useVideoContext } from '../../store/VideoContext'; | ||
|
|
||
| function VideoDetailPage() { | ||
| const { state } = useVideoContext(); |
There was a problem hiding this comment.
Tip: You can get extra fancy with destructuring if you'd lke
const { state: {currentVideo}} = useVideoContext();
and then you can use currentVideo.videoThumbnail below
| }); | ||
|
|
||
| return ( | ||
| <VideoContext.Provider value={{ state, dispatch }}>{children}</VideoContext.Provider> |
There was a problem hiding this comment.
it would be better to expose helper functions like setCurrentVideo and getFavoriteVideos instead of state and dispatch directly.
3 participants
Link to app deployed: https://gracious-heyrovsky-f2f814.netlify.app/
###Features accomplished
###Features missing