Video Details View - Mini Challenge 3 #47
Conversation
There was a problem hiding this comment.
I made a few suggestions, I would love to see more styled-components even though you use Material UI. Let me know if you need help with this.
Also, I see a high coverage but not real unit tests. That's not ideal bc they are not helping you out to understand the code.
There are various files deleted, just make sure you are not removing main components for other functionalities.
I saw it working in a stable manner but you should think in support your code with good unit tests.
| ------------------------------|----------|----------|----------|----------|-------------------| | ||
| File | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s | | ||
| ------------------------------|----------|----------|----------|----------|-------------------| | ||
| All files | 84.21 | 50 | 88.89 | 82.35 | | |
|
|
||
| describe('App', () => { | ||
| it('renders App component', () => { | ||
| render(<App />); |
There was a problem hiding this comment.
Hmm 🤔 , this test is expecting nothing, this would increase your coverage but not really testing anything else than it is rendering with no errors
|
|
||
| describe('Navbar', () => { | ||
| it('renders Navbar component', () => { | ||
| render(<Navbar />); |
|
|
||
| console.log(videoInfo); | ||
|
|
||
| const classes = useStyles(); |
There was a problem hiding this comment.
you are not passing a theme object, so I think that's the reason you are missing more than several styles at your components. Do not want to try styled-components instead?
|
|
||
| const classes = useStyles(); | ||
| const { title, channelTitle, thumbnails } = videoInfo.snippet; | ||
| const { videoId } = videoInfo.id; |
There was a problem hiding this comment.
| const { videoId } = videoInfo.id; | |
| const { id: videoId } = videoInfo; // looks better in my opinion |
There was a problem hiding this comment.
even better if you integrate it with the previous line
| } | ||
| } | ||
|
|
||
| export default withStyles(useStyles)(CardVideo); |
There was a problem hiding this comment.
it is a good practice to leave an empty line at EOF
|
|
||
| this.classes = props.classes; | ||
| this.videoInfo = props.videoInfo; | ||
| this.videoId = (typeof this.videoInfo.id === 'object') ? this.videoInfo.id.videoId : this.videoInfo.id; |
There was a problem hiding this comment.
This comparison is not secure due to the variants of results of typeof. Instead you could try:
| this.videoId = (typeof this.videoInfo.id === 'object') ? this.videoInfo.id.videoId : this.videoInfo.id; | |
| const { id = {} } = props.videInfo; // provide a default value - could be numeric too | |
| this.videoId = id.videoId || id; // if not an object with that videoId property, it takes the value provided for id or the default in your code |
| const response = await axios.get( | ||
| 'https://www.googleapis.com/youtube/v3/search', { | ||
| params: { | ||
| part: 'snippet', | ||
| q: search, | ||
| maxResults: 20, | ||
| type: 'video', | ||
| key: process.env.REACT_APP_YOUTUBE_API_KEY, | ||
| } | ||
| } | ||
| ) |
There was a problem hiding this comment.
Don't you think this could be refactored into a single fn that then is called at the two fetch.. functions?
Also, the code is not error handling for HTTP errors nor for wrong expected data
| <Container maxWidth={false}> | ||
| <Grid container justify="center" spacing={3}> | ||
| {videos.map((video) => { | ||
| let videoId = (typeof video.id === 'object') ? video.id.videoId : video.id; |
There was a problem hiding this comment.
if you're executing the same validation every time, this could be extracted to an external utility
| import { Link, useHistory } from 'react-router-dom'; | ||
|
|
||
| export default function Navbar(props){ | ||
| const classes = useStyles(); |
There was a problem hiding this comment.
you could destructure the properties and use them directly at the JSX code
| const classes = useStyles(); | |
| const { root, menuButton, navbarTitle, ... etc } = useStyles(); |
No description provided.