Skip to content

Code Review#2

Open
fabiolmorato wants to merge 1 commit intobrunatb:mainfrom
fabiolmorato:review
Open

Code Review#2
fabiolmorato wants to merge 1 commit intobrunatb:mainfrom
fabiolmorato:review

Conversation

@fabiolmorato
Copy link

No description provided.

Copy link
Author

@fabiolmorato fabiolmorato left a comment

Choose a reason for hiding this comment

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

O projeto ficou bem escrito, organizado e legível. Levantei alguns pontos por comentários pelo Pull Request. Em geral, o grupo mostrou domínio dos conceitos apresentados em aula.

let mounted = true;
const request = axios.get('https://mock-api.bootcamp.respondeai.com.br/api/v1/linkr/users/follows', token);
request.then(response =>{
if(mounted)verifyFollowers(response.data.users);
Copy link
Author

Choose a reason for hiding this comment

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

Poderia ter colocado um espaço depois do parêntese do if, ficaria ainda mais legível :)

const request = axios.get('https://mock-api.bootcamp.respondeai.com.br/api/v1/linkr/users/follows', token);
request.then(response =>{
if(mounted)verifyFollowers(response.data.users);
}).catch(() => alert('erro'))
Copy link
Author

Choose a reason for hiding this comment

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

Este alert poderia ter uma mensagem mais descritiva. Não indica muito ao usuário ou ao desenvolvedor saber que aconteceu um erro sem especificá-lo.

Comment on lines -18 to -20
<Link to='/my-posts' onClick={() => setPage(0)}><li>My posts</li></Link>
<Link to='/my-likes'><li>My likes</li></Link>
<Link to='/'><li onClick={clearLocal}>Logout</li></Link>
Copy link
Author

Choose a reason for hiding this comment

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

Semanticamente, os filhos de um ul deveriam ser li. Por isso, Link deveria estar dentro de li, e não o contrário.

}

function verifyLink(data){
return data.includes('youtube');
Copy link
Author

Choose a reason for hiding this comment

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

Esta lógica cobre mais casos que o desejado. Um link como http://en.wikipedia.org/wiki/youtube daria true.

const [text, setText] = useState('');
const [geoLocation, setGeoLocation] = useState({});
const { token, user } = useContext(UserContext);
const [enable, setEnable] = useState(false);
Copy link
Author

Choose a reason for hiding this comment

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

Este estado tem o nome enable e é utilizada para controlar se um botão está desabilitado. Poderia se chamar disable.

Comment on lines -53 to -60
<InfiniteScroll
dataLength={posts.length}
next={() => {
setPage(page + 10);
}}
hasMore={posts.length < 10 ? false : true}>
{(posts.map((post) => <Post key={post.id} post={post} />))}
</InfiniteScroll>
Copy link
Author

Choose a reason for hiding this comment

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

Como este componente tem mais de uma linha, recomenda-se colocá-lo entre parênteses. Não colocar parênteses pode causar erros estranhos.

}

return(
<EditContext.Provider value={{postEdit, editClick, editing, setEditing, textEdit, setTextEdit, postId, setPostId, modified, setModified, disabled, setDisabled}}>
Copy link
Author

Choose a reason for hiding this comment

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

Este objeto passado no value poderia ter sido dividido em linhas, para que ficasse mais legível e fácil de encontrar as partes.

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.

1 participant

Comments