Skip to content

Code Review#1

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

Code Review#1
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 no Pull Request. Reparei que às vezes não havia ponto-e-vírgula ao final de algumas linhas e alguns arquivos contém mais de uma linha em branco em sequência. Em geral, o grupo mostrou domínio nos conceitos apresentados em aula.

Comment on lines -4 to -5
Switch,
Route
Copy link
Author

Choose a reason for hiding this comment

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

Como esses componentes não foram utilizados neste arquivo, não precisavam ser importados.


return (
<Top>
<Link to='/timeline' onClick={()=>setPage(0)}><p>linkr</p></Link>
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 mais espaços na função do onClick: () => setPage(0). Deixaria o código ainda mais legível :)

export default function Login({setTask}){
const [email, setEmail] = useState('');
const [password, setPassword] = useState('');
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 poderia ter um nome mais significativo, como loggingIn, já que seu valor depende de estar esperando a requisição de login terminar ou não.

import UserContext from '../contexts/UserContext';


export default function Posts(props) {
Copy link
Author

Choose a reason for hiding this comment

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

Por questões semânticas, como este componente representa somente um post, o componente deveria se chamar Post, não Posts.

<Profile>
<Link to={(user.user.id == id) ? '/my-posts' : `/user/${id}`}
onClick={()=>setPage(0)}>
<img src={avatar} /></Link>
Copy link
Author

Choose a reason for hiding this comment

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

O fechamento da tag Link poderia ter sido feito na linha abaixo. Pode prejudicar a leitura do código, pois parece que falta um nível de indentação na linha abaixo.

Comment on lines -48 to -50
onChange={e => setLink(e.target.value)}
value={link}
disabled={enable} />
Copy link
Author

Choose a reason for hiding this comment

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

Estas linhas poderiam ter um nível a mais de indentação, para deixar claro que são continuação do elemento.

Comment on lines -70 to -82
display:flex;
justify-content:center;

img{
width:60px;
border-radius:10px;
}
`;

const Text = styled.p`
color:#FFF;
text-align:center;
font-size:20px;
Copy link
Author

Choose a reason for hiding this comment

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

A falta de espaços entre a propriedade e o valor pode diminuir a legibilidade. Por isso, recomendo colocar um espaço em branco sempre após os dois pontos.


export default function Trending() {
const { userToken, setPage } = useContext(UserContext);
const [ hashtags, setHashtags ] = useState(null);
Copy link
Author

Choose a reason for hiding this comment

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

Como o valor de hashtags depois se torna um array, recomendo que o valor inicial seja um array vazio. Evita erros inesperados no código, por conta de algum trecho que espera que seja um array.

<h2>save, share and discover<br />the best links on the web</h2>
</section>
<StyledSection>
{task ? <Login setTask={setTask} /> : <SignIn setTask={setTask} />}
Copy link
Author

Choose a reason for hiding this comment

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

Cuidado com a nomenclatura dos componentes. Login e SignIn significam a mesma coisa, pode causar confusão. O SignIn deveria se chamar SignUp.

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