Feature/mini challenge3 #2
Conversation
wdonet
left a comment
wdonet
left a comment
There was a problem hiding this comment.
You're right, I could not make it work at Codesandbox but did it on my local 🎉
All functionalities work fine except that when selecting a new video from the list and you're viewing one already. That list of related videos is not updated with the new clicks.
I left a few comments. You have a great opportunity to add better unit test thus increase your code coverage.
Good job though. You made it 👏🏽 - please do the adjustments as you consider and merge later to continue to the next challenge.
Please ask any question.
| ------------------------------|----------|----------|----------|----------|-------------------| | ||
| File | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s | | ||
| ------------------------------|----------|----------|----------|----------|-------------------| | ||
| All files | 61.61 | 50 | 50 | 62.16 | | |
There was a problem hiding this comment.
This is good enough for now, thanks for adding the file!
| return ( | ||
| <BrowserRouter> | ||
| <AuthProvider> | ||
| <NavBar word={word} setWord={setWord} /> |
There was a problem hiding this comment.
It seems word is used only inside NavBar component, so maybe it is more useful inside the component than here.
You can always send the setVideos function to NavBar as you did with setword for saving the videos into this context
| }); | ||
|
|
||
| it('renders all children', () => { | ||
| const layout = screen.getByTestId('layout'); |
There was a problem hiding this comment.
Layout component renders a main html tag. This is reachable by role : main. Thus the data-testid is not necessary.
| const handleChange = (event) => { | ||
| setSearchWord(event.target.value); | ||
| }; |
There was a problem hiding this comment.
Using destructuring could be simpler:
| const handleChange = (event) => { | |
| setSearchWord(event.target.value); | |
| }; | |
| const handleChange = ({ target: { value } }) => setSearchWord(value); |
| it('renders SideMenu and its elements', () => { | ||
| const sideMenu = screen.getByTestId('sideMenu'); | ||
| expect(sideMenu.children.length).toEqual(1); | ||
| expect(sideMenu.children[0].children.length).toEqual(2); |
There was a problem hiding this comment.
You can enrich your tests by looking for specific components like buttons, input texts or content that can be changed based on the arguments passed to the component.
There was a problem hiding this comment.
Testing only for the number of children inside the main container at each component could be the cause of a low coverage.
|
|
||
| beforeEach(() => { | ||
| render( | ||
| <BrowserRouter> |
There was a problem hiding this comment.
I'm not sure you need to use BrowserRouter here
| {videos.map((video) => | ||
| video.id.videoId ? ( | ||
| <Col key={video.id.videoId}> | ||
| <VideoCard key={video.id.videoId} video={video} /> | ||
| </Col> | ||
| ) : ( | ||
| '' | ||
| ) | ||
| )} |
There was a problem hiding this comment.
Could improve doing:
- using the provided index by map fn at the key prop for the
Colcomponent this case - The second key prop is not necessary, it is used at Col only.
| {videos.map((video) => | |
| video.id.videoId ? ( | |
| <Col key={video.id.videoId}> | |
| <VideoCard key={video.id.videoId} video={video} /> | |
| </Col> | |
| ) : ( | |
| '' | |
| ) | |
| )} | |
| {videos.map((video, index) => | |
| video.id.videoId ? ( | |
| <Col key={index}> | |
| <VideoCard video={video} /> | |
| </Col> | |
| ) : ( | |
| '' | |
| ) | |
| )} |
| @@ -0,0 +1,17 @@ | |||
| const key = 'AIzaSyCbnwRIBvWCoQGaLIOFUx4rhLHg-Sb1a8s'; | |||
There was a problem hiding this comment.
😱
Do not save your api key at public repositories, it is better if you handle it with environment variables. Check dotenv for example.
| export async function fetchVideos(keyWord) { | ||
| const apiBaseUrl = `https://content-youtube.googleapis.com/youtube/v3/search?part=id&part=snippet&maxResults=25&q=${keyWord}&key=${key}`; | ||
|
|
||
| return fetch(apiBaseUrl) |
There was a problem hiding this comment.
What if this fn is a custom hook instead ? Do you see a better/cleaner usage for this functionality at the other components using it?
2 participants
No description provided.