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

Nova lib para geração das thumbnails #1425

Merged

Conversation

ErickCReis
Copy link
Contributor

Resolve #782
Continua implementação do PR #818

@vercel
Copy link

vercel bot commented May 25, 2023

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

A member of the Team first needs to authorize it.

@ErickCReis ErickCReis force-pushed the new-og-image-generation branch from a1386b8 to 5da8f89 Compare May 25, 2023 00:53
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.

Boa @ErickCReis, obrigado por atualizar o PR! 💪

Não vamos ter a grande vantagem da biblioteca da Vercel porque não conseguimos acessar nosso banco de dados da Edge, certo? Então sabe dizer que outras vantagens teremos com essa mudança? E teremos alguma desvantagem?

Os problemas do PR #559 estão sendo corrigidos?

Fiz também alguns comentários no código 👍

Comment on lines +1 to +2
/* eslint-disable jsx-a11y/alt-text */
/* eslint-disable @next/next/no-img-element */
Copy link
Collaborator

Choose a reason for hiding this comment

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

Se não forem muitos pontos problemáticos, é melhor desativar apenas na linha em questão, pois assim ficam mais claras as decisões de ignorar o lint.

Copy link
Contributor Author

@ErickCReis ErickCReis May 25, 2023

Choose a reason for hiding this comment

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

Desativar apenas na linha foi minha primeira tentativa mas como é um trecho de jsx a única solução que encontrei foi:

{// eslint-disable-next-line jsx-a11y/alt-text, @next/next/no-img-element
}<img
  src="..."
  width={82.5}
  height={66}
  style={{ marginLeft: 40 }}
/>

Além de ficar muito estranho, se usar a formatação do Prettier esse trecho é alterado e o comentário não funciona:

{
  // eslint-disable-next-line jsx-a11y/alt-text, @next/next/no-img-element
}
<img
  src="..."
  width={82.5}
  height={66}
  style={{ marginLeft: 40 }}
/>

import content from 'models/content.js';
import removeMarkdown from 'models/remove-markdown';

async function asPng(contentObject) {
const parentContentObject = await content.findOne({
Copy link
Collaborator

Choose a reason for hiding this comment

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

Provavelmente a maioria dos conteúdos compartilhados em redes sociais são "root", então compensa verificar se existe parent_id antes de fazer essa consulta.

Comment on lines +19 to +20
width: 1200,
height: 630,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Se não me engano, existem dimensões que funcionam melhor do que outras em algumas redes, mas não sei quais são os valores. A mudança leva isso em consideração? Eu também não sei se os valores anteriores consideravam o que era melhor para alguma rede específica.

Obs. Sei que a Vercel recomenda usar 1200x630, mas será legal se der para saber o motivo.

Copy link
Contributor Author

@ErickCReis ErickCReis May 25, 2023

Choose a reason for hiding this comment

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

Procurei um pouco a respeito disso e não achei muita informação, o único consenso foi a proporção da imagem ser 1.91:1.

Esse novo valor (1200x630) é aparentemente o mínimo recomendado por todos os sites que busquei, acho que faz sentido utilizar o valor mínimo porque o conteúdo ainda é exibido sem nenhum problema e a imagem é mais "leve".

package.json Outdated Show resolved Hide resolved
@ErickCReis
Copy link
Contributor Author

@aprendendofelipe

Não vamos ter a grande vantagem da biblioteca da Vercel porque não conseguimos acessar nosso banco de dados da Edge, certo?

Na estrutura atual não é possível utilizar a lib @vercel/og justamente por conta disso, a documentação diz que ela funciona exclusivamente no Edge Runtime.

Use Edge Runtime by enabling the runtime: 'edge' config flag as the default Node.js runtime is not supported

Estive pensando um pouco e acho que seria possível utilizar o Edge Runtime se a gente passasse todas a informações necessárias pela url. Seria um mudança na estrutura das thumbnails, não se isso traria mais vantagens ou desvantagens, mas posso investigar isso um pouco mais a fundo se for o caso.


Os problemas do PR #559 estão sendo corrigidos?

Sim


Então sabe dizer que outras vantagens teremos com essa mudança? E teremos alguma desvantagem?

Além de resolver o problema com as thumbnails, acho que todo código é muito mais simples e fácil de manter. Não identifiquei nenhuma desvantagem além da adição de mais uma lib.


Fiz também alguns comentários no código 👍

Beleza!

@vercel
Copy link

vercel bot commented May 26, 2023

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

Name Status Preview Comments Updated (UTC)
tabnews ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 26, 2023 0:15am

@aprendendofelipe aprendendofelipe merged commit f236e12 into filipedeschamps:main May 26, 2023
@aprendendofelipe
Copy link
Collaborator

aprendendofelipe commented May 26, 2023

Show @ErickCReis, já está em produção! 🚀

E problema antigo resolvido.

image

Muito obrigado por mais essa contribuição! 💪

@ErickCReis ErickCReis deleted the new-og-image-generation branch May 26, 2023 01:12
@filipedeschamps
Copy link
Owner

Muuuito massa turma!!! 🎉 🎉 🎉

E em alguns casos deu para perceber um grande benefício em como as palavras se encaixam melhor 🤝 🤝 🤝

image

@aprendendofelipe
Copy link
Collaborator

Agora que percebi que no Antes/Depois que eu coloquei o "antes" era um link para a thumbinal e não um print, então após revalidação do cache acabou ficando igual ao novo 😅

Mas relembrando o problema, essa thumb ficava com o título sobreposto ao logo do TabNews

@filipedeschamps
Copy link
Owner

Mas relembrando o problema, essa thumb ficava com o título sobreposto ao logo do TabNews

kkkk massa!!!!

@aprendendofelipe
Copy link
Collaborator

@ErickCReis você foi citado no post comemorativo:

https://www.tabnews.com.br/FelipeBarso/tabnews-novo-ranqueamento-otimizacoes-e-mais 🎉

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.

Criar og:image automaticamente usando a nova biblioteca da vercel
3 participants