Skip to content

Conversation

@amandarangel
Copy link

Esse é um PR de correção. Não é necessário mergear.

Copy link
Author

@amandarangel amandarangel left a comment

Choose a reason for hiding this comment

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

Bom dia Felipe e Sandro!
O projeto foi avaliado como: dentro do esperado. Parabéns pela entrega!
Tenho alguns pontos para vocês levarem em consideração: 1) Pensar com carinho na nomeção das variáveis. Ex: no seu state, para guardar um array de mensagens, você nomearam a propriedade como mensagem; por ser um array e conter múltiplas mensagens, o mais correto é que o nome fique no plural. Pode parecer picuinha, mas conforme a aplicação for crescendo, a consistência na nomeação de variáveis toma uma importância bem grande. 2) A ideia de apagar a mensagem no doubleClick era retirar a mensagem do array e não só apagar o seu texto. Uma sugestão, seria vocês filtrarem o array e retirarem a mensagem que foi clicada. 3) Esse ponto tem a ver com o anterior: vi que vocês colocaram como key do p da mensagem, a própria mensagem. O ideal seria vocês colocarem como key algo que vai ser único (pois mais de uma mensagem pode ter usuário e texto idênticos). No caso dessa aplicação que não tinha um id, uma ideia seria usar como key o index daquela mensagem no array. 4) Vocês componentizaram bonitinho. Se eu fosse dar só mais uma ideia seria tornar cada mensagem como um componente separado também.
No mais, fiquei muito feliz que vocês fizeram a componentização e fizeram um arquivo separado para o styled-components. O código ficou bem organizadinho! :)

@@ -0,0 +1,70 @@
# Getting Started with Create React App
Copy link
Author

Choose a reason for hiding this comment

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

Pessoal, uma coisa muito legal e comum no mercado é a utilização do README para colocar informações sobre o projeto. Então incentivamos vocês a editarem esse README padrão do React. O link do surge, inclusive, deve ficar aqui! :)

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.

4 participants