-
Notifications
You must be signed in to change notification settings - Fork 77
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
Challenge3 - Hooks #50
base: master
Are you sure you want to change the base?
Conversation
is missing the input value
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.
Comments
Good Job Aure! I really like the code styling and how you structure it, it has the expected React styling.
I can see that you extracted the style of the components, nice!
I saw the .env ignore rule, nice!
Please help us with the creation of PR into your own repo, currently, you're doing the PR into my repo, so I won't able to approve it when done.
Don't worry I didn't mark any of the checkboxes (it was difficult to determine which feature was done), we have time!
I encourage you to start doing the test, because, without them, we are not able to deliver the certificate.
I encourage you to ask for help if you're stuck, bored or have some questions.
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).
import { MenuButton, MenuContainer } from './Menu.styles'; | ||
|
||
import './Menu.styles.js'; |
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.
If you're importing the components, there's no need to import the file Menu.styles.js
.
So it would be only
import { MenuButton, MenuContainer } from './Menu.styles';
const Menu = () => (
.....
|
||
const submit = e => { | ||
e.preventDefault(); | ||
console.log(value) |
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 recommend removing all the console.log function calls before the PR creation
src/utils/constants.js
Outdated
export default { | ||
AUTH_STORAGE_KEY: 'wa_cert_authenticated', | ||
API_URL: 'https://www.googleapis.com/youtube/v3/', | ||
API_KEY: 'AIzaSyA2RrmXuKEobD96oL8gnqZz1Mh52iBUNd8', |
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 never uploading secrets to the Github repo, so I suggest adding the API_KEY
into the .env
file. Also, I recommend creating a .env.example, track the file, and put there the necessary variables
.env
REACT_APP_API_KEY=<value_of_the_key>
src/utils/constants.js
...
export default {
...,
API_KEY: process.env.REACT_APP_API_KEY,
}
.env.sample
REACT_APP_API_KEY=
Mini-Challenge 3: Hooks
The Challenge
Acceptance Criteria
Bonus Points