-
Notifications
You must be signed in to change notification settings - Fork 396
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
Adiciona tabela de conteúdos em publicações #1684
Conversation
@wendesongomes is attempting to deploy a commit to the TabNews Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Muito obrigado pelo PR! É uma ótima contribuição para o projeto.
Deixei algumas anotações em partes específicas do código, mas acredito que temos duas melhorias principais que podem ser feitas:
- Usar o
Dialog
do Primer para exibir a tabela de conteúdos, que já possui um comportamento e estilização compatível com nossa UI. Exibir a tabela de conteúdos de outra forma também pode ser uma opção, como o GitHub faz (Exibir tabela de conteúdos (links para os títulos) na publicação #1679 (comment)), mas como ninguém se posicionou contra, acredito que podemos seguir com o comportamento proposto inicialmente.
Se esse comportamento ficar bem encapsulado, não deve ser difícil trocar oDialog
por outro componente. - Obter os títulos usando alguma biblioteca como o
mdast-util-toc
, que é utilizada peloremark-toc
. Acredito que seria necessário criar um plugin próprio Remark para usá-lo como um plugin ByteMD, e ao invés de inserir a tabela de conteúdos no conteúdo, disponibiliza-la para ser acessada por outro componente.
Não sei quão complicado ou factível é seguir o que proponho nesse ponto 2.
PS: Para manter atualizada com a main
, recomendo utilizar o comando git rebase main
(com a main
atualizada), assim não terá um commit específico como aconteceu em a9b643c por causa do merge.
import { IconButton } from '@/TabNewsUI'; | ||
import { ChevronUpIcon } from '@/TabNewsUI/icons'; | ||
|
||
export default function GoToTopButton() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Não acho que faz sentido remover o componente GoToTopButton
, mesmo que surja a necessidade de ter dois botões flutuantes. Usá-los deveria ser algo tão simples como:
return (
<Box style={...}>
<AnotherButton />
<GoToTopButton />
</Box>
);
E, se for necessário criar um componente FloatingButton
, então ele pode renderizar os filhos recebidos ao invés de precisar conhecê-los.
function FloatActionButton({ children }) {
return (
<Box style={...}>
{children}
</Box>
)
}
function ChangeModal() { | ||
setShowTopics(!showTopics); | ||
document.body.style.overflow = showTopics ? 'scroll' : 'hidden'; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No React, funções que começam com uma letra maiúscula são componentes. Não é o caso aqui.
Além disso, não me parece ser necessário mudar o estilo dessa forma. Além de estar referenciando um valor antigo de showTopics
(usar setShowTopics
só irá mudar o valor de showTopics
a partir da próxima renderização), como esse valor é um estado, pode ser acessado durante a renderização.
justifyContent: 'start', | ||
paddingX: '60px', | ||
transform: 'translate(-50%, -50%)', | ||
zIndex: 99, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Não deveria ser necessário usar o zIndex
assim. Ele deve seguir uma lógica crescente de prioridade: 1
, 2
, 3
... Dificilmente precisará de valores maiores do que esses em algum projeto.
Além disso, tem muitos estilos nesse Box
. Considerando preocupações com acessibilidade e transições, seria ótimo usar um componente do Primer para não nos preocuparmos com esses detalhes de implementação. Investigando a documentação, encontrei o Dialog, veja um exemplo usando ele na lateral no Storybook.
@@ -26,70 +26,74 @@ export default function Post({ contentFound, rootContentFound, parentContentFoun | |||
} | |||
}, []); | |||
|
|||
const topics = contentFound.body.match(/^#+\s*(.*)$/gm); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usar um RegEx abre muita margem para obter os dados de forma errada. Veja esse exemplo:
E o texto usado:
#
<!-- #título comentado -->
# <!-- comentário no título --> ok teste <b>formatação html</b> <i>italico</i>
# Título 1
<h1>Título 1 html</h1>
### Título 3
## Título 2
##### Título 6
##### Título 6 parte 2 bem comprido vamos ver o que acontece quando ele é super longo yay
#### Título 4
##### Título 5
Outro ponto: da forma que está, ao editar o conteúdo e abrir a lista, ela não é atualizada.
key={index} | ||
sx={{ marginLeft: `${title.match(/#+/).toString().split('').length * 2}` }}> | ||
<Link | ||
href={`#user-content-${title |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Montar a âncora assim não é o ideal, já que usamos um plugin que é responsável por gerar o id
, ou seja, precisaríamos ter cuidado com todo esse tratamento.
@Rafatcb tentei fazer um plugin para pegar os titulos porem tive muito problema de varios re-renders indesejado, então pesquisando mais um pouco cheguei a essa conclusão:
gostaria de saber o q acha desta forma? como ele faz uma div com a class .markdown-body, estou pegando todos os h e observando, resolvendo o problema do usuário, mudar algo no titulo e o dialog n alterar os títulos, conseguir usar o dialog do prime. |
Mudanças realizadas
Adiciona um botão com índice do post:
Quando aberto fica desta forma:
Resolve #1679
Tipo de mudança