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

Remove markdown do body do conteúdo por dentro do getStaticProps #661

Merged
merged 5 commits into from
Aug 22, 2022

Conversation

kaique-soares
Copy link
Contributor

Este PR tem como objetivo resolver a issue #650

Com esse PR nós passamos a utilizar o model removeMarkdown por dentro do getStaticProps nos lugares em que o componente DefaultLayout recebe o conteúdo pela propriedade content, passando a limpar o body do conteudo pelo server-side.

A implementação em si me parece simples, no entando, uma dificuldade que tive foi comprovar que a modificação que eu fiz, de fato, ocorreu em todos os lugares que devia. E esse é um ponto que peço a ajuda de vocês, se realmente o único lugar em que o DefaultLayout recebe a propriedade content é no arquivo pages/[username]/[slug]/index.public.js 👍

@vercel
Copy link

vercel bot commented Aug 19, 2022

@kaique-soares is attempting to deploy a commit to the TabNews Team on Vercel.

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

Copy link
Owner

@filipedeschamps filipedeschamps left a comment

Choose a reason for hiding this comment

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

@kaique-soares muito obrigado pela contribuição!!!!!! Eu não rodei o código, mas fiz umas sugestões, veja se faz sentido 🤝

@@ -240,6 +241,9 @@ export async function getStaticProps(context) {

const secureChildrenList = authorization.filterOutput(userTryingToGet, 'read:content:list', childrenFound);

const cleanBody = removeMarkdown(secureContentValues.body).replace(/\s+/g, ' ');
secureContentValues.body = cleanBody;
Copy link
Owner

Choose a reason for hiding this comment

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

Se você sobrescrever o body com o valor do cleanBody, será que isso não vai remover o markdown dos conteúdos da página? Eu não rodei aqui e estou dando esse palpite só pela leitura do código, posso estar bastante enganado 🤝

De qualquer forma, estou pensando aqui, talvez podemos criar uma terceira variável que é retornada no props lá em baixo, por exemplo:

      contentFound: JSON.parse(JSON.stringify(secureContentValues)),
      childrenFound: JSON.parse(JSON.stringify(secureChildrenList)),
      sanitizedMetadata: ...

E alí você já colocaria todo o metadado sanitizado, incluindo com o .substring(0, 190) por exemplo no caso do body, ou o do title e todas as informações que são enviadas aqui:

<DefaultLayout content={contentFound}>

Então ao invés de enviar o contentFound, enviar o sanitizedMetadata, que é um objeto muito similar ao contentFound, mas só com os dados que o DefaultLayout precisa e todos já sanitizados/trabalhados.

Copy link
Owner

Choose a reason for hiding this comment

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

Outro detalhe é fazer o rebase e resolver o conflito 🤝

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Se você sobrescrever o body com o valor do cleanBody, será que isso não vai remover o markdown dos conteúdos da página? Eu não rodei aqui e estou dando esse palpite só pela leitura do código, posso estar bastante enganado 🤝

Nos testes em localhost não houve perda da formatação mas especulo que tenha sido um falso positivo, justamente por eu ter testado apenas em localhost, por causa do getStaticProps. Vou investigar e testar em homologação 🤝

De qualquer forma, estou pensando aqui, talvez podemos criar uma terceira variável que é retornada no props...

Aaah sensacional 😍 Vou implementar, e pelo ângulo que eu tava olhando nunca teria pensado nisso, apesar de ter visto essa abordagem no passado, na playlist dos robôs! 🤯

Outro detalhe é fazer o rebase e resolver o conflito 🤝

👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

rebase feito e conflito resolvido 👍

E com isso, destaco alguns pontos:

  1. Para resolver o conflito e não quebrar nada, voltamos a utilizar o model removeMarkdown como antes, por dentro do DefaultLayout, então, no fim das contas a dinâmica de limpar o body do conteúdo pelo client-side continua e é isso que vou atacar agora, trabalhando na questão do sanitizedMetadata que tu sugeriu 👍
  2. Sobre o rebase, nunca havia utilizado o comando antes, conhecia mas não havia aplicado ainda, então, não sei se foi a estratégia mais adequada, começar por ele ou ter continuado a implementação e feito o rebase depois.

Copy link
Owner

Choose a reason for hiding this comment

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

Sobre o item 2, eu sempre prefiro fazer o rebase antes para deixar a branch limpa de merge commits o máximo que der 👍 mas não existe o certo ou errado, são estratégias com trade-offs diferentes 🤝

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@filipedeschamps após o commit, em que trabalhei na implementação do sanitizedMetadata, fui investigar o ponto que tu levantou:

Se você sobrescrever o body com o valor do cleanBody, será que isso não vai remover o markdown dos conteúdos da página?...

E sim, foi um falso positivo:

Nos testes em localhost não houve perda da formatação mas especulo que tenha sido um falso positivo, justamente por eu ter testado apenas em localhost, por causa do getStaticProps. Vou investigar...

E comprovei isso forçando o SWR usar o fallbackData: contentFoundFallback, quebrando a url da mesma postagem que antes "não havia perdido a formatação", porque ela perdia mas eu que não percebi, foi um detalhe sutil que me deu uma rasteira sensacional 😅 e vou compartilhar isso no TabNews 🤝

content-body-without-markdown

@kaique-soares kaique-soares force-pushed the remove-markdown-by-server-side branch from 06f3c29 to 568cad3 Compare August 20, 2022 13:51
@filipedeschamps
Copy link
Owner

@aprendendofelipe acompanha essa delicinha de PR, continuando sua implementação sobre a performance das páginas 🤝

@kaique-soares kaique-soares force-pushed the remove-markdown-by-server-side branch from 82910c0 to be9c29f Compare August 20, 2022 19:14
@vercel
Copy link

vercel bot commented Aug 21, 2022

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

Name Status Preview Updated
tabnews ✅ Ready (Inspect) Visit Preview Aug 21, 2022 at 10:21PM (UTC)

@aprendendofelipe
Copy link
Collaborator

Opa, boa iniciativa @kaique-soares!

Como não existe nenhuma necessidade de recomputar os metadados após o render inicial, a minha ideia era mover para dentro de getStaticProps qualquer lógica necessária para montar os metadados, não somente o remove-markdown, mas toda a lógica que está dentro do componente DefaultLayout.

Então eu começaria sem mudar nada que já existe, mas só adicionando um model metadata que fosse capaz de lidar com todas as variações de metadados que temos para as diferentes páginas e retornar tudo que precisamos: type, title, description, image, url, noIndex, author, published_time, modified_time.

Em seguida eu criaria um componente MetaTags, que vai ser bem parecido com o arquivo BaseLayout, mas com um nome mais esclarecedor.

Com isso feito, aí começaria a mudar todas as páginas para usar o model metadata dentro de getStaticProps, que vai passar a propriedade metadata para a página, que por sua vez irá repassar para o componente MetaTags.

E o componente BaseLayout não será mais necessário em DefaultLayout.

Acho que era essa minha ideia, se fizer sentido pra você ou se quiser partir por outro caminho, posso ir lhe ajudando 🤝

@filipedeschamps
Copy link
Owner

@aprendendofelipe não consegui visualizar muito bem, mas algumas coisas eu capturei 🤝

Sugestão: eu só faria uma movimentação assim depois que a RFC de layouts do Next.js for aprovada e esse novo recurso entrar em produção, pois eu especulo que iremos jogar tudo que temos hoje fora. Então eu vou seguir com esse PR altamente sugiro para reformularmos tudo somente após a RFC. Esse merge também vai me liberar para mandar um PR que estou quase terminando aqui e que implementa um método que encontra o conteúdo root a partir do child e que vai conflitar com as mudanças aqui. Vai ser possível dar muito mais contexto para os emails de notificação e também para as páginas individuais de respostas 🤝

@aprendendofelipe
Copy link
Collaborator

@filipedeschamps, será que não é melhor então deixar esse PR para depois? Pois já está bastante remendada a parte de metadados, e vamos remendar ainda mais ao invés de aproveitar para organizar a casa?

Só um exemplo <DefaultLayout content={sanitizedMetadata}>.

Mas se quiser encerrar hoje esse PR, posso dar uma refatorada enquanto você cuida do PR que encontra o root a partir do child

@filipedeschamps
Copy link
Owner

Show! 😍

Então vou continuar aqui com a implementação e que também se tiver conflito, vai ser super simples de resolver 🤝

@aprendendofelipe
Copy link
Collaborator

Está feito tudo que eu tinha pensado em fazer 😅

Do que eu tinha listado acima, só não fiz o model, mas na verdade por enquanto não precisa, pois só a rota \[username]\[slug] iria usar.

@filipedeschamps
Copy link
Owner

Agora entendi o que você queria fazer @aprendendofelipe 😂 👍

Talvez o nome BaseLayout e DefaultLayout não ficaram claros, mas a estratégia com eles era a seguinte:

  • BaseLayout: isto era para ser de fato a base/sustentação de qualquer layout. Não deveria ter lógica alguma aqui sobre como tratar os dados e apenas lógica sobre como pegar os dados que foram enviados e colocar nas metatags do cabeçalho. Ele então é um layout sem layout, ele é um "layout técnico", vamos colocar assim. Isso possibilitaria a criação de vários tipos de layout em cima dele, sem que eles se preocupem com a parte das metatags.
  • DefaultLayout: então esse foi o primeiro layout que foi criado em cima do BaseLayout e ele se responsabiliza por tratar dos dados e definir de fato uma estrutura de layout (por exemplo, largura responsiva). Talvez aqui a gente poderia ter dividido ele em dois layouts na verdade: ListLayout e ContentLayout, pois esse DefaultLayout acabou sendo um padrãozão mesmo que foi usado em todos os casos, incluindo nos de formulário e daí lá ficou um pouco estranho reduzir o tamanho do container.

Então isso possibilita, por exemplo, termos o FullWidthLayout que seria um layout que preenche toda a largura da página caso a gente queira fazer uma "landing page" ou algo do tipo, onde ela possa ter suas características únicas e não se preocupar com metadados, incluindo nem ter a obrigatoriedade do title terminar com "· TabNews".

Mas eu li no PR por cima, porque apesar de tecnicamente eu já ter conseguido fazer tudo sobre o root content aqui, eu não to conseguindo nem a pau deixar bom o UX das novas informações na página do conteúdo. Já montei e remontei aqui mil vezes, to quase tendo um treco 😂 mas uma hora eu vou conseguir 🤝

@aprendendofelipe
Copy link
Collaborator

Mudei a lógica de acrescentar o · TabNews no final do title para não ter risco de ficar algo como · TabNews · TabNews

@kaique-soares
Copy link
Contributor Author

@aprendendofelipe muiiiito obrigado! 😍

Acessei o Github para nofiticar o @filipedeschamps sobre o progresso de alguns pontos levantados por ele:

@kaique-soares muito obrigado pela contribuição!!!!!! Eu não rodei o código, mas fiz umas sugestões, veja se faz sentido handshake

e fui direto para a conversation e depois fui ler os comentários 👍

E foi simplismente sensacional o que eu li, muito obrigado!

Bom, com toda certeza eu levaria muito mais tempo para implementar o que tu propôs e já implementou 😅

De qualquer forma, valeu muito a experiência e tudo que aprendi até aqui 👍 e vou tentar implementar localmente mesmo assim 😅

@aprendendofelipe
Copy link
Collaborator

Talvez o nome BaseLayout e DefaultLayout não ficaram claros

Eu não tinha entendido justamente por não ter nada de Layout no BaseLayout 😅

Se a questão é ter uma base padrão para as tags, tem uma alternativa mais comum que é passar a configuração padrão para o _app.public.js e criar um componente para ser usado nas páginas em que as tags forem diferentes.

Acho que podemos usar os nomes DefaultHead e Head.

Acabei de subir essa alteração para você avaliar.

Eu mantive o Head no componente DefaultLayout (no lugar do MetaTags que eu tinha criado). Assim não precisamos mudar nada nas páginas, mas o mais usual é colocar o Head com seus argumentos em cada página.

Do jeito que eu fiz irá funcionar os dois jeitos, ou seja, colocando o Head na página ou passando os metadados para o DefaultLayout. Vai valer o que for passado por último.

E o Head também está bem versátil, pois aceita a propriedade metadata no mesmo padrão que o DefaultLayout, mas também aceita qualquer configuração extra via children, por exemplo:

<Head><title>Título da página</title></Head>

ou

<Head metadata={{ title: 'Título da página' }} />

ou

<DefaultLayout metadata={{ title: 'Título da página' }} />

tem todos o mesmo efeito


@aprendendofelipe muiiiito obrigado! 😍

Imagina, e se tiver qualquer coisa que você faria diferente, vamos conversando.

@aprendendofelipe
Copy link
Collaborator

Todas as tags estão sendo geradas como deveriam.

Aproveitei para colocar o título na única página que não tinha, que era a de ativação do cadastro. E agora todas as páginas tem descrição padrão, caso não seja informada uma específica.

E a performance da página 😍😍😍

Produção

image

Essa branch

image

Ps. A diferença de SEO é pq em homologação a Vercel não deixa indexar as páginas

Conclusão

Ainda podemos melhorar bastante, mas estamos no caminho 🚀

Copy link
Collaborator

@aprendendofelipe aprendendofelipe left a comment

Choose a reason for hiding this comment

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

@filipedeschamps, deixei os dois commits separados para ser fácil de reverter para a versão que você achar melhor. Mas agora que entendi sua intenção com o BaseLayout, ou seja, que você queria garantir a presença das tags em qualquer página que seja criada futuramente, então a última versão é a que garante isso. Se aprovar eu faço squash dos dois últimos commits.

@filipedeschamps
Copy link
Owner

@aprendendofelipe desculpa a demora em responder!! Vida real aqui me atravessou 😂

E cara que solução sensacional do componente DefaultHead e essa é a garantia que precisamos sim 🤝

E sugiro deixar todos os commits, ta um histórico MUITO massa! Então nesse ponto não sugiro apagar nada e vou fazer merge 👍

@filipedeschamps filipedeschamps merged commit ba5acdf into main Aug 22, 2022
@filipedeschamps filipedeschamps deleted the remove-markdown-by-server-side branch August 22, 2022 00:11
@filipedeschamps
Copy link
Owner

Mergeeeed!! Let's goooooo!!! Parabéns @kaique-soares e @aprendendofelipe !

E @kaique-soares viu como valeu a pena abrir o PR? A turma aqui do TabNews é muito massa!! 🤝

@filipedeschamps
Copy link
Owner

@aprendendofelipe de onde exatamente é esse report? Seria show testar agora como ficou 🤝

@aprendendofelipe
Copy link
Collaborator

@aprendendofelipe desculpa a demora em responder!! Vida real aqui me atravessou

Opa, mas é essa a vida que importa! haha

@aprendendofelipe de onde exatamente é esse report? Seria show testar agora como ficou 🤝

É do web.dev https://web.dev/measure

Em produção a pontuação dessa publicação pesada foi de 44 para 73 🚀

image

Estou testando com essa publicação que tem cópia nos dois ambientes:

https://www.tabnews.com.br/jackson541/models-e-consultas-em-django

Sempre rodo duas vezes para garantir que já exista cache na região do servidor do teste.

@filipedeschamps
Copy link
Owner

foi de 44 para 73 🚀

WOOOOOWW 😍 que sensacional!!!!! Muito muito bom!!!

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.

3 participants