Skip to content

Conversation

@Fannypc
Copy link

@Fannypc Fannypc commented Mar 20, 2021

Cambios correspondientes a "Mini-Challenge 1: Core Concepts and Styling"

Copy link

@vmcruz vmcruz left a comment

Choose a reason for hiding this comment

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

Looks good overall, just added a couple of comments. Let me know if you want some help with the husky issues. Please also change your base branch to master, instead of the one you forked the project from.

Comment on lines -49 to -59
"lint-staged": {
"*.{js, jsx, css, json}": [
"yarn run lint:fix",
"pretty-quick --staged",
"git add"
]
},
"husky": {
"hooks": {
"pre-commit": "lint-staged"
}
Copy link

Choose a reason for hiding this comment

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

What were the problems with husky?, I might be able to help you with that.

import styled from 'styled-components';


const CardContainer = styled.div`
Copy link

Choose a reason for hiding this comment

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

I suggest to put your styled components into its styled.js file, this way you'll have a more cleaner component. You could do something like:

// styled.js
export const CardContainer = styled.div`
  ...
`

// CardVideo.component.jsx
import { CardContainer } from './styled';

import React from 'react';
import styled from 'styled-components';
import { FontAwesomeIcon } from '@fortawesome/react-fontawesome'
import { faBars, faUser } from '@fortawesome/free-solid-svg-icons'
Copy link

Choose a reason for hiding this comment

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

I recommend to import the fontawesome icons using the Icon Library approach, this way your bundle get smaller. This is what fontawesome suggests:

https://fontawesome.com/how-to-use/javascript-api/setup/importing-icons

Once imported, you can use FontAwesomeIcon with the prop icon='name' to actually use it.

function Navbar() {

return (
<>
Copy link

Choose a reason for hiding this comment

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

Since Header is your root component here, you don't need to use fragments. Also check the comment about moving your styled components outside your main component.

<>
<Navbar/>
<CardsContainer>
{listVideos.items.map((item)=>{return <CardVideo key={item.etag} item={{item}}/>})}
Copy link

Choose a reason for hiding this comment

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

You could omit the return if you do:

{listVideos.items.map((item) => <CardVideo key={item.etag} item={{item}} />)}

Also, I think you added extra {} to your item prop. That's why you are doing props.item.item in your CardVideo component.

`;


function CardVideo(props){
Copy link

Choose a reason for hiding this comment

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

You can extract the props using destructuring, so it is cleaner in your component, like so:

function CardVideo({ item }) {

Check the comment about the extra {} cause I think it wasn't intended.

Copy link

Choose a reason for hiding this comment

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

Now that you are using props, it could be useful to put in place a set of rules for those props, so that the component expects exactly what you meant. Check the documentation for these and let me know if you have any questions:

https://reactjs.org/docs/typechecking-with-proptypes.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants