Ana Vazquez - mini-Challenge 3 #10
Conversation
There was a problem hiding this comment.
Comments
Nice touch with the CSS! I see little padding on all the app, it could be great if you were able to fix it
Also, the expected behavior on the VideoDetails when you click a related video, the related videos change based on the new displayed video. (Your related videos are not changing)
I recommend not removing eslint and prettier, those libraries help you and us to maintain styling of coding with best practices.
I see the first row a different size from the others.
Please the next deliverable create the PR into your own repository so that way you could integrate the code into your own repo. The current PR won't be able to merge it, because you're comparing it to another base repo.
Final comment: please add your coverage report, so I can verify the coverage percentage.
From all the reviewed challenges (more than 30) you're the first one to rerender the content on the card, awesome job! Keep it up.
Acceptance Criteria
- Videos in the Home View are fetched from the YouTube API and displayed correctly.
- Users can search for YouTube videos using the search field on the header.
- A video can be played from the Video Details View after clicking on it.
- The video information and related videos list are displayed correctly on the Video Details View.
- When a user clicks on a related video the video player, information and related videos list are updated.
Bonus Points
- Custom Hooks for API calls are implemented and tested correctly.
- Testing coverage is above 60%. (Please include a screenshot of the code coverage report in your PR description).
| { | ||
| "extends": ["react-app", "airbnb", "prettier", "prettier/react"], | ||
| "plugins": ["prettier"], | ||
| "rules": { | ||
| "prettier/prettier": "error", | ||
| "react/jsx-filename-extension": "error", | ||
| "react-hooks/exhaustive-deps": "warn", | ||
| "import/no-unresolved": ["off", { "ignore": [".css$"] }], | ||
| "import/prefer-default-export": "off", | ||
| "react/destructuring-assignment": "off", | ||
| "react/jsx-props-no-spreading": "off", | ||
| "react/prop-types": "off", | ||
| "array-callback-return": "off", | ||
| "consistent-return": "off", | ||
| "no-console": "off" | ||
| } | ||
| } |
There was a problem hiding this comment.
I would recommend not removing this file, instead try a little bit to make it work with your IDE
| { | ||
| "trailingComma": "es5", | ||
| "tabWidth": 2, | ||
| "printWidth": 90, | ||
| "semi": true, | ||
| "singleQuote": true, | ||
| "jsxSingleQuote": false, | ||
| "arrowParens": "always" | ||
| } |
There was a problem hiding this comment.
I would recommend not removing this file, instead try a little bit to make it work with your IDE
| @@ -0,0 +1,22 @@ | |||
| { | |||
There was a problem hiding this comment.
I recommend adding the route build/ on your .gitignore, it is not a good practice to keep track of build destination directories (they will change very often)
| @@ -1,38 +1,30 @@ | |||
| { | |||
| "name": "react-certification-2020", | |||
There was a problem hiding this comment.
Could I ask why did you removed the starting point of the app? the installed packages were helping you for the formatting. It is not a bad option removing all the content, is just curiosity.
| let history = useHistory(); | ||
| function playVideo(event) { | ||
| history.push(`/${videoId}`); | ||
| console.log(title); |
There was a problem hiding this comment.
I recommend removing all the console.logs from all your files before the PR creation
| function ListVideos({title, description,url, videoId, videoList, setVideoData}) { | ||
|
|
||
|
|
||
| let history = useHistory(); |
There was a problem hiding this comment.
You could use const becuase you are not going to change the reference of the variable, keeping all your variables that won't change the value with const will help you to keep the function pure (without side effects)
src/VideoItem.jsx
Outdated
|
|
||
| function VideoItem({title, description,url,videoId, setTitles, setVideoList,searchResults}) { | ||
|
|
||
| //const [results, setResults] = React.useState([]); |
There was a problem hiding this comment.
I recommend removing commented code before the PR creation, so we are sure that we don't introduce commented code into our codebase
| //const [results, setResults] = React.useState([]); | ||
|
|
||
|
|
||
| let history = useHistory(); |
src/VideoItem.jsx
Outdated
| console.log(title); | ||
| console.log(setTitles({title : title, | ||
| description: description, | ||
| videoId: videoId, | ||
| })); | ||
| setVideoList(searchResults); | ||
| } |
src/header.jsx
Outdated
| if (event.key === 'Enter') { | ||
| submitValue() | ||
| } | ||
| } | ||
|
|
||
| const submitValue = () => { | ||
| console.log(value); |
Mini-Challenge 3