Refatoração do componente `Content` #378

filipedeschamps · 2022-05-24T03:24:05Z

PR para forçar deploy em Preview do PR #370 feito por @aprendendofelipe

refactor(Content): and minor fixes in localStorage

Refatoração do componente `Content`

vercel · 2022-05-24T03:24:32Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
tabnews	✅ Ready (Inspect)	Visit Preview	May 24, 2022 at 3:25AM (UTC)

…tion correction Re-reading the function to open the editor when reloading the page if it is saved in localStorage. Function correction to avoid reloading them

vercel · 2022-05-24T03:53:40Z

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

To accomplish this, @andreghisleni 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.

andreghisleni · 2022-05-24T03:58:09Z

Opaaa desculpa só ter enviado agora, eu só modifiquei as funções para evitar o recarregamento desnecessário das funções e adicionei novamente a verificação de que se existe conteúdo salvo no localstorage ele carregue o editor

@filipedeschamps

filipedeschamps · 2022-05-24T04:09:54Z

@andreghisleni por favor, peço que rode um npm run lint:fix para o commit possa passar no CI 🤝

Run lint:fix

aprendendofelipe

Oi @andreghisleni, não estava mais abrindo automaticamente o editor por questão de performance e também sugestão do @filipedeschamps aqui: #364 (comment)

Talvez seja interessante removermos o recurso de automaticamente abrir o modo de edição se for encontrado algo no localStorage e só se focar em carregar essas informações quando entrar nesse modo de fato, seja pelo clique no botão, ou seja por esse ser o modo padrão de um conteúdo root. O que acham?

Acho que a sugestão foi mais por causa do bug que estava ocorrendo naquele momento, mas pensei na questão de performance eu achei válida.
Por isso o acesso ao localStorage tinha sido movido para dentro do componente EditMode, para só ocorrer um acesso nos componentes já renderizados em modo de edição (ao clicar em "editar", "responder" ou Publicar novo conteúdo") ao invés de ocorrer um acesso a cada "conteúdo" ou "botão de responder" que for renderizado.

Pela experiência do usuário eu acho super válida a funcionalidade de abrir o modo de edição automaticamente sempre que carregar um conteúdo que contenha rascunho no loacalStorage, mas acredito que seja melhor essa decisão de iniciar o componente em modo de edição ser tomada por quem chama o componente Content, não dentro dele, pelo custo que a leitura do localStorage traz.
No caso de conteúdo root novo, ele sempre é carregado em modo de edição, então já vai ser carregado o rascunho.

No restante das alterações eu não percebi nenhum impacto positivo em utilizar useCallback nessas funções, já que elas não são passadas para outros componentes. Se tem algo que eu não estou enxergando, me dê uma clareada, por favor.

Por enquanto acredito que a versão do link abaixo está legal:
https://tabnews-git-refactor-content-preview3-tabnews.vercel.app/

Added check if the user is the owner of the publication to not check for all contents

andreghisleni · 2022-05-25T13:21:09Z

Pronto agora adicionei a verificação de que se a publicação é do próprio usuário antes de buscar no localstorage.

Agora a questão de utilizar o useCallback é principalmente questão de boas praticas, alem de evitar o recarregamento desnecessário

aprendendofelipe

Edit: Tinha colocado resultados incorretos nos testes.

Agora a questão de utilizar o useCallback é principalmente questão de boas praticas, alem de evitar o recarregamento desnecessário

@andreghisleni, o useCallback evita a redefinição da função a cada renderização do componente onde ela é definida. Isso é mais útil para não causar o recarregamento de outros componentes que dependem da função. Mas em EditMode não temos outros componentes que dependem dessas funções, então a redefinição não causa nenhuma renderização extra.

Se quando diz recarregamento você está falando da redefinição das funções, então sim, isso pode ser reduzido mesmo no nosso caso. Fiz um teste para ter números para comparar os casos com e sem useCallback.

Utilizei esse código:

...
var funcCount = new Set()

export default function Content({ content, mode = 'view', viewFrame = false }) {
...
...
  function EditMode() {
...
...
    funcCount.add(loadLocalStorage);
    funcCount.add(clearErrors);
    funcCount.add(handleSubmit);
    funcCount.add(handleChange);
    funcCount.add(handleCancel);
    console.log("Count: ", funcCount.size);
...

Número de redefinições de funções em cada caso

Commit	`ee49e9e`	`0245a73`	_
Ação \ useCallback	Sem	Com	Funções redefinidas com useCallback
Ao clicar em responder	5	5	Definição inicial de todas as 5
Ao navegar para publicar novo conteúdo	15	15	Cada uma 3 vezes
No primeiro dígito escrito	10	2	handleSubmit e hancleCancel
A cada dígito extra	5	2	handleSubmit e hancleCancel
Ao voltar o foco para a aba	5	2	handleSubmit e hancleCancel

Imagino que cada redefinição seja menos custosa sem o useCallback, então para as renderizações iniciais o caso sem useCallback deve ser um pouco mais rápido. Mas, após o carregamento, durante a digitação do conteúdo, ocorrem menos redefinições das funções com o useCallback. Então isso me convence de que é melhor utilizá-lo.

Só vou dar uma analisada melhor depois se não podemos diminuir as variáveis no array de dependências de alguma delas. Acho que consigo fazer isso amanhã e dou um retorno aqui.

Agora se está lhe incomodando muito o fato de não estar entrando automaticamente no modo de edição, acho mais interessante fazer essa mudança em outro componente. Vamos conversar mais sobre isso.🤝

aprendendofelipe · 2022-05-25T18:22:37Z

pages/interface/components/Content/index.js

-    const localStorageContent = localStorage.getItem(localStorageKey);
-    if (isValidJsonString(localStorageContent)) {
-      setComponentMode('edit');
+    if (user?.id && !isLoading && contentObject?.owner_id === user?.id) {


Pronto agora adicionei a verificação de que se a publicação é do próprio usuário antes de buscar no localstorage.

Opa @andreghisleni, isso não funciona para todos os casos! Só vai abrir o modo de edição para as alterações de conteúdos já postados. Não vai funcionar para rascunhos de novas postagens em resposta de outro conteúdo.

Pois é eu percebi que não funciona para todos os casos, mas pelo menos na edição ele funciona

Então vamos deixar assim, funcionando parcialmente e futuramente podemos corrigir.

andreghisleni · 2022-05-25T20:08:18Z

Realmente faz sentido eu acho que acabei me confundindo com o caso do useCallback

filipedeschamps · 2022-05-25T20:14:11Z

@aprendendofelipe como estou focado nas tarefas do backend, peço que feche esse PR e abra um na versão que devo fazer o merge na main e deploy. Depois disso podemos fazer outras melhorias nesse assunto, mas sugiro nos focarmos em outras que acredito ter mais impacto, seja as issues da milestone ou issues como essa que de fato é uma ótima ideia.

aprendendofelipe · 2022-05-25T20:46:40Z

@andreghisleni e @filipedeschamps me perdoem, mas o primeiro teste tinha um erro enorme. Editei a resposta com o resultado correto e comentários atualizados.

vercel · 2022-05-25T20:54:22Z

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

To accomplish this, @aprendendofelipe 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.

aprendendofelipe

@filipedeschamps considerando a revisão dos testes, pode fazer o merge na main com todos os commits dessa PR.

O @andreghisleni colocou de volta a funcionalidade de abrir a edição automaticamente quando encontrar rascunho no localStorage para o caso de edição de conteúdo já postado. Só não abre automaticamente para novas respostas deixadas em rascunho, mas se clicar em responder, o rascunho estará lá.

filipedeschamps · 2022-05-25T21:21:05Z

Show!! Feito o merge e o PR novo é este: #385

aprendendofelipe and others added 9 commits May 22, 2022 19:06

refactor(Content): and minor fixes in localStorage

4c7eaef

Merge pull request #367 from filipedeschamps/refactor-content

5891bf9

refactor(Content): and minor fixes in localStorage

feat(edit content): allow delete source_url

c78df17

fix(source_url validator): prev. version restored

44ececc

fix(remove source_url): source_url = null

7fca342

Merge pull request #368 from filipedeschamps/refactor-content-preview

c8a152f

Refatoração do componente `Content`

feat(post submit): !username -> router('/login')

1f9f07d

feat(content component): no warning that you need to be logged in

ee49e9e

Merge pull request #370 from filipedeschamps/refactor-content-preview2

dfd18ae

Refatoração do componente `Content`

vercel bot deployed to Preview May 24, 2022 03:25 View deployment

refactor(content): rereading the function to open the editor and Func…

abb177d

…tion correction Re-reading the function to open the editor when reloading the page if it is saved in localStorage. Function correction to avoid reloading them

andreghisleni requested a review from aprendendofelipe May 24, 2022 04:04

refactor(content): run lint

2230633

Run lint:fix

aprendendofelipe reviewed May 24, 2022

View reviewed changes

refactor(content): added check if the user is the owner

0245a73

Added check if the user is the owner of the publication to not check for all contents

andreghisleni requested a review from aprendendofelipe May 25, 2022 13:22

aprendendofelipe reviewed May 25, 2022

View reviewed changes

Merge branch 'main' into refactor-content-preview3

a41a7d5

aprendendofelipe approved these changes May 25, 2022

View reviewed changes

filipedeschamps changed the base branch from main to refactor-content-preview4 May 25, 2022 21:19

filipedeschamps merged commit 8372bb2 into refactor-content-preview4 May 25, 2022

filipedeschamps deleted the refactor-content-preview3 branch May 25, 2022 21:20

filipedeschamps mentioned this pull request May 25, 2022

Refatoração do componente Content #385

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refatoração do componente `Content` #378

Refatoração do componente `Content` #378

filipedeschamps commented May 24, 2022

vercel bot commented May 24, 2022 •

edited

vercel bot commented May 24, 2022

andreghisleni commented May 24, 2022 •

edited

filipedeschamps commented May 24, 2022

aprendendofelipe left a comment •

edited

andreghisleni commented May 25, 2022 •

edited

aprendendofelipe left a comment •

edited

aprendendofelipe May 25, 2022

andreghisleni May 25, 2022

aprendendofelipe May 25, 2022

andreghisleni commented May 25, 2022

filipedeschamps commented May 25, 2022

aprendendofelipe commented May 25, 2022

vercel bot commented May 25, 2022

aprendendofelipe left a comment

filipedeschamps commented May 25, 2022

Refatoração do componente Content #378

Refatoração do componente Content #378

Conversation

filipedeschamps commented May 24, 2022

vercel bot commented May 24, 2022 • edited

vercel bot commented May 24, 2022

andreghisleni commented May 24, 2022 • edited

filipedeschamps commented May 24, 2022

aprendendofelipe left a comment • edited

Choose a reason for hiding this comment

andreghisleni commented May 25, 2022 • edited

aprendendofelipe left a comment • edited

Choose a reason for hiding this comment

Número de redefinições de funções em cada caso

aprendendofelipe May 25, 2022

Choose a reason for hiding this comment

andreghisleni May 25, 2022

Choose a reason for hiding this comment

aprendendofelipe May 25, 2022

Choose a reason for hiding this comment

andreghisleni commented May 25, 2022

filipedeschamps commented May 25, 2022

aprendendofelipe commented May 25, 2022

vercel bot commented May 25, 2022

aprendendofelipe left a comment

Choose a reason for hiding this comment

filipedeschamps commented May 25, 2022

Refatoração do componente `Content` #378

Refatoração do componente `Content` #378

vercel bot commented May 24, 2022 •

edited

andreghisleni commented May 24, 2022 •

edited

aprendendofelipe left a comment •

edited

andreghisleni commented May 25, 2022 •

edited

aprendendofelipe left a comment •

edited