Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(interface): #745 respostas duplicadas #749

Closed
wants to merge 1 commit into from
Closed

fix(interface): #745 respostas duplicadas #749

wants to merge 1 commit into from

Conversation

Rafaelb4rros
Copy link

Aparentemente o problema era que, quando um novo
conteúdo era criado no componente Content, os estados
componentMode e contentObject eram modificados e nunca voltavam
para o seu estado inicial (recebidos por props), o conteúdo duplicado
se dava quando o usuário trocava de página e voltava, o que acabava mostrando
o conteúdo criado verdadeiro que vinha do backend e o mesmo conteúdo
porém renderizado devido ao estado do Content não resetar.

Eu fiz duas coisas:

A primeira foi resetar o contentMode de volta
ao estado inicial quando o usuario troca de página, isso resolveu
um problema que era o conteudo ficar duplicado.

O segundo problema era que o contentObject
permanecia com os dados anteriores, e quando mudava de página e
voltava, ao clicar no botão responder o input vinha preechido com essas
informações, o que não faz sentido, já que o usuário não estava editando o
conteúdo e sim criando um novo. Para resolver isso, eu criei uma função
que reseta o contentObject e passei ela para o componente CompactMode
que é responsável por renderizar o botão de resposta.

@vercel
Copy link

vercel bot commented Sep 26, 2022

@Rafaelb4rros is attempting to deploy a commit to the TabNews Team on Vercel.

To accomplish this, @Rafaelb4rros needs to request access to the Team.

Afterwards, an owner of the Team is required to accept their membership request.

If you're already a member of the respective Vercel Team, make sure that your Personal Vercel Account is connected to your GitHub account.

@Rafaelb4rros
Copy link
Author

Parece que o CI não está rodando, sera que fiz algo errado?

@33gustavo33
Copy link
Contributor

Parece que o CI não está rodando, será que fiz algo errado?

No meu também não rodou, acho que é o que o Filipe falou na parte Além do roteiro o que mais precisa ter no commit nesse comentário aqui

@Rafaelb4rros
Copy link
Author

Parece que o CI não está rodando, será que fiz algo errado?

No meu também não rodou, acho que é o que o Filipe falou na parte Além do roteiro o que mais precisa ter no commit nesse comentário aqui

Acho que é isso, eu não rodei o 'npm run commit'. Vou tentar reverter esse commit atual e rodar o comando, acho que isso vai resolver (eu espero kkkkk).

@Rafaelb4rros
Copy link
Author

Usei todas as minhas habilidades e não consegui fazer o CI rodar... e agora?

@filipedeschamps
Copy link
Owner

@Rafaelb4rros muito obrigado por tentar de tudo para fazer rodar o CI! Infelizmente isso é um bug randômico causado pela integração da Vercel aqui no GitHub. Já reportei isso para eles que confirmaram o bug mas não há nenhum prazo (ou diria até incentivo) para consertar isso. Nesses casos, o CI só irá rodar para quem tem permissão de fazer deploy.

Mas não se preocupe, assim que eu revisar o PR, irei fazer o merge numa branch intermediária que irá fazer rodar tudo, incluindo o deploy no ambiente de homologação 🤝

Copy link
Collaborator

@Rafatcb Rafatcb left a comment

Choose a reason for hiding this comment

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

Esse é meu primeiro contato com o código do TabNews, mas reparei um problema no código em relação a como o React trabalha com hooks.

@@ -71,7 +72,7 @@ export default function Content({ content, mode = 'view', viewFrame = false }) {
if (componentMode === 'view') {
return <ViewMode setComponentMode={setComponentMode} contentObject={contentObject} viewFrame={viewFrame} />;
} else if (componentMode === 'compact') {
return <CompactMode setComponentMode={setComponentMode} />;
return <CompactMode resetContentObject={() => setContentObject(content)} setComponentMode={setComponentMode} />;
Copy link
Collaborator

@Rafatcb Rafatcb Sep 27, 2022

Choose a reason for hiding this comment

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

A dependência de um hook está sendo redeclarada em cada render: como o resetContentObject é utilizado como dependência de um hook no CompactMode, o fato dele não ser declarado com useCallback torna o useCallback do CompactMode inútil (nessa situação):

  • Solução 1: Declarar o resetContentObject com um useCallback ao invés de uma função comum.
  • Solução 2: Remover o useCallback do CompactMode. Não sei qual a necessidade dele. À primeira vista, parece ser apenas para que o Button não seja re-renderizado, o que me parece uma otimização desnecessária.

Encontrei que esse hook foi adicionado no commit fix(content component): changes requested in the pull request (PR #344), mas ainda não vi a necessidade dele. Na minha opinião, a solução 2 é a mais adequada. Fico no aguardo do review do Filipe e de outras pessoas 👍

Copy link
Author

Choose a reason for hiding this comment

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

De fato, a passagem da função para o CompactMode dessa forma vai tornar o useCallback inútil nesse caso.

Também não encontrei nenhum motivo para utilização do hook dentro do CompactMode, já que é um componente extremante simples aonde a unica função dele é renderizar o botão e não vai ter praticamente nenhum impacto se a função handleClick for recriada e ele for re-renderizado.
Também dei uma olhada em todos os lugares aonde o hook useCallback é utilizado e parece que o unico caso aonde ele é desnecessário é esse.

Acho interessante a segunda solução de remover o useCallback do componente CompactMode.

@aprendendofelipe
Copy link
Collaborator

Bom trabalho @Rafaelb4rros, me ajudou a perceber que a solução é simplesmente reiniciar o estado do componente que renderiza a resposta.

Então propus outra maneira fazer isso, passando uma key para o React saber quando reiniciar o componente.

Está no PR #760. Se puderem testar, está no ar em:

https://tabnews-git-comments-bug-tabnews.vercel.app/

Valeu!

@Rafaelb4rros
Copy link
Author

Rafaelb4rros commented Oct 6, 2022

Bom trabalho @Rafaelb4rros, me ajudou a perceber que a solução é simplesmente reiniciar o estado do componente que renderiza a resposta.
Então propus outra maneira fazer isso, passando uma key para o React saber quando reiniciar o componente.
Está no PR #760. Se puderem testar, está no ar em:
https://tabnews-git-comments-bug-tabnews.vercel.app/
Valeu!

hahaha genial. Olhando como o Content é rendezirado eu nunca pensaria que seria possível passar essa propriedade. Foi minha primeira vez vendo um componente sendo renderizado recursivamente assim. Realmente sua solução é muito mais simples e correta, fico feliz em ter ajudado. Essa foi a primeira contrubuição que fiz para algum projeto open source e espero contribuir muito mais para o tabnews.

Eu já posso fechar essa PR ou deixo aberta?

@aprendendofelipe
Copy link
Collaborator

espero contribuir muito mais para o tabnews.

Boa! A Turma aqui é demais! 🤩

Eu já posso fechar essa PR ou deixo aberta?

Se preferir aguardar para ter certeza de que o #760 está 100%, então é só ficar de olho no repositório e fechar se ele for pra produção. Ou também dá pra fechar e qualquer coisa reabrir 🤝


Falando em fechar e reabrir o PR, eu vou fazer isso agora aqui só para testar se esse procedimento força a rodar os testes, pois aparentemente isso funcionou em um outro PR, mas queria confirmar 😉

@aprendendofelipe aprendendofelipe changed the base branch from main to beta October 6, 2022 17:41
@aprendendofelipe aprendendofelipe changed the base branch from beta to main October 6, 2022 17:41
@aprendendofelipe
Copy link
Collaborator

Falando em fechar e reabrir o PR, eu vou fazer isso agora aqui só para testar se esse procedimento força a rodar os testes, pois aparentemente isso funcionou em um outro PR, mas queria confirmar 😉

Aqui não adiantou fechar e reabrir o PR para forçar o CI 🙈

@filipedeschamps
Copy link
Owner

Trabalho sensacional da comunidade! Estou fechando esse PR em favor do PR #760 e citar os dois PRs na issue que está listada na Milestone 5 🤝

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.

None yet

5 participants