-
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
Wizeline_project #26
base: master
Are you sure you want to change the base?
Wizeline_project #26
Conversation
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.
Hi Jesus, some how the project was deployed by Vercel in this URLs
- react-certification-2020-jduarthe.vercel.app
- react-certification-2020-xi.vercel.app
- react-certification-2020-git-master-jduarthe.vercel.app
I think you last commit fixed the issue. I like what you have up to this point, I left some comments most are little things, if you need something just ping me in slack, great work 👍
src/components/CardList/CardList.jsx
Outdated
|
||
useEffect(() => { | ||
fetchData(); | ||
console.log(data) |
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 see a log in production 😬, it has happened to me 😆
src/components/Card/Card.jsx
Outdated
const property = { | ||
imageUrl: item.snippet.thumbnails.default.url, | ||
imageAlt: "Rear view of modern home with pool", | ||
title: item.snippet.channelTitle, | ||
reviewCount: 34, | ||
rating: 4, | ||
} |
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.
Here you are getting the item, which is the video, in the props, I'm not sure why you are hardcoding the some attributes of item, the imageAlt
, reviewCount
, and rating
, also the title you are using is the title of the channel instead of the channel of the video. It is not a big deal since all you have to change the attributes to use the correct properties
const fetchData = async () => { | ||
const res = await fetch("https://gist.githubusercontent.com/jparciga/1d4dd34fb06ba74237f8966e2e777ff5/raw/f3af25f1505deb67e2cc9ee625a633f24d8983ff/youtube-videos-mock.json"); | ||
return res; | ||
} |
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 function is also defined in the file CardList.jsx
in line 8, you can create a utils folder and move this function to that folder and import the same function wherever you need, this way you don't have duplicated code and in the future you can maintain it easily
import "@testing-library/jest-dom/extend-expect"; | ||
import Title from "./Title"; | ||
|
||
describe('Testing Title component', () => { |
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 is also a minor thing, in the describe title I see that you have a variation of titles, sometimes you have a title like Testing CardList
, or NavBar
or Testing Title component
, you can use a format to put the title in your tests to be consistent, when a project grows it helps in the long run.
You can always use a title like {name what you are testing} {type of what you are testing, i.e. component, or hook, or context provider, or a utility function, etc}
, for example:
CardList Component
NavBar Component
Title Component
useTheme Hook
Theme Provider
- etc
Co-authored-by: PacoMojica <[email protected]>
Co-authored-by: PacoMojica <[email protected]>
Co-authored-by: PacoMojica <[email protected]>
Co-authored-by: PacoMojica <[email protected]>
Co-authored-by: PacoMojica <[email protected]>
Hi this is my wizeline_project