Skip to content
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

branch Hooks create view video and related videos #3

Merged
merged 2 commits into from
Aug 4, 2021
Merged

Conversation

7honarias
Copy link
Owner

in this challenge,

  • separed styled in a new file for each component
  • make some more test
  • include axios for fetch api youtube
  • new page view for show video and related videos

test_challenges

@netlify
Copy link

netlify bot commented Aug 2, 2021

✔️ Deploy Preview for naughty-franklin-3633f3 ready!

🔨 Explore the source changes: 9979c7f

🔍 Inspect the deploy log: https://app.netlify.com/sites/naughty-franklin-3633f3/deploys/61082068036a290008880ba3

😎 Browse the preview: https://deploy-preview-3--naughty-franklin-3633f3.netlify.app/

Copy link

@dianaatwizeline dianaatwizeline left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice PR! Keep up the good job


describe('VideoGrid', () => {
it('Should render Text wizeline', () => {
const { getByText } = render(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of destructuring the return of the render method, it is better to use screen.getByText

source: Common Mistakes with React Testing Library

if (error !== null) {
console.log(error);
}
// eslint-disable-next-line react-hooks/exhaustive-deps

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is almost always wrong to use this rule. Right here I think you added it to not add fetchRelation and fetchVideo, the definition of those functions should not be changing, and if they are and triggering the useEffect callback to run more times than needed then we need to to something with those functions instead of adding this disable.

Let me know if you have questions on this as this took me a long time to fully wrap my head around this

}
}, [value, data, fetchSearch, error]);

const enterPressed = async (event) => {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you have two different enterPressed that I think could be only one function living inside the Header component, so instead of passing enterPressed you can pass value and move enterPressed to the Header component

@7honarias 7honarias merged commit f45e10f into master Aug 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants