Exibir tabela de conteúdos (links para os títulos) na publicação #1686
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.
Não cheguei a testar como ficou na prática após essas alterações, mas deixei alguns comentários. A maioria são coisas simples. Foram boas alterações 🤝
Se me surgir alguma ideia de solução melhor para o uso do componente e para a identificação dos títulos, eu crio outro comentário aqui.
| const [isOpenDialogTopics, setOpenDialogTopics] = useState(false); | ||
| const returnFocusRefDialogTopics = useRef(null); | ||
|
|
||
| // set overflow on body to 'hidden' when dialog is open |
There was a problem hiding this comment.
Não precisa desse comentário, pelo código dá para entender o que é feito.
| import { Head } from 'pages/interface'; | ||
|
|
||
| export default function DefaultLayout({ children, containerWidth = 'large', metadata }) { | ||
| const [isOpenDialogTopics, setOpenDialogTopics] = useState(false); | ||
| const returnFocusRefDialogTopics = useRef(null); |
There was a problem hiding this comment.
Pelo o que entendi, isso não está sendo usado, certo? Ainda não testei após as alterações usando o Primer para ver se o foco está sendo retornado corretamente.
| setHeadings(Array.from(updatedHeadingsElements)); | ||
| }; | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
Por motivos de organização, e também tem um pouco a ver com as regras dos hooks, é melhor chamar todos os hooks antes de declarar outras funções.
| import { Box, Dialog } from '@primer/react'; | ||
| import { useEffect, useRef, useState } from 'react'; | ||
|
|
||
| import { Link } from '@/Link'; |
There was a problem hiding this comment.
No projeto temos um padrão para importar elementos visuais pelo @/TabNewsUI, isso facilita a substituição quando necessário. Então, ficaria assim:
| import { Box, Dialog } from '@primer/react'; | |
| import { useEffect, useRef, useState } from 'react'; | |
| import { Link } from '@/Link'; | |
| import { useEffect, useRef, useState } from 'react'; | |
| import { Box, Dialog, Link } from '@/TabNewsUI'; |
E se você está usando um componente novo que ainda não é exportado pelo TabNewsUI/index.js, como é o caso do Dialog, basta adicionar a exportação lá (está em ordem alfabética).
| margin: 0, | ||
| left: 'auto', | ||
| right: 0, | ||
| maxHeight: '100vh', | ||
| height: '100vh', | ||
| transform: 'translate(0)', | ||
| overflow: 'auto', | ||
| padding: 4, | ||
| '@media screen and (max-width: 750px)': { | ||
| maxHeight: '100vh', |
There was a problem hiding this comment.
Aqui me parece que ainda tem mais estilos do que precisa. Não cheguei a testar para ver quais podem ser removidos, mas reparei que o maxHeight é declarado com o mesmo valor duas vezes, por exemplo. Tem algum motivo para usar a media query aqui, ou era algo que você estava testando e esqueceu de remover?
There was a problem hiding this comment.
o maxHeight do media coloquei e esqueci de tirar, mas eu preciso colocar esses estilos para resetar o do próprio dialog para colocar do lado direito, pois o q esta na documentação n esta funcionando
| @@ -0,0 +1,39 @@ | |||
| import { ListUnorderedIcon } from '@primer/octicons-react'; | |||
There was a problem hiding this comment.
Já comentei sobre a importação de elementos visuais, mas no caso de ícones, importamos de @/TabNewsUI/icons.
| const updatedHeadingsElements = document.querySelectorAll( | ||
| '.markdown-body h1, .markdown-body h2, .markdown-body h3, .markdown-body h4, .markdown-body h5, .markdown-body h6', | ||
| ); |
There was a problem hiding this comment.
Essa solução está melhor do que o Regex, apesar de ainda ter alguns problemas. Seria interessante conseguirmos uma solução melhor. Vi que esse componente é usado no DefaultLayout, e por isso acho que não está sendo feito da melhor maneira, já que todas as páginas terão esse componente que não necessariamente faz sentido.
Não estou falando que você precisa resolver isso sozinho, é que ainda não tive uma ideia para isso. Se outra pessoa estiver vendo esse PR, pode contribuir também.
There was a problem hiding this comment.
sim ele fica no defaultLayout, mas pelo o que eu vi o .markdown-body só existe nas postagens mesmo, mas vou dar uma olhada de como melhorar esse código, sobre as importações erradas, por algum motivo ele não importou do lugar certo e eu n tinha percebido, e muito obrigado pelas dicas
| headings.length > 0 && ( | ||
| <IconButton | ||
| variant="invisible" | ||
| aria-label="Retornar ao topo" |
There was a problem hiding this comment.
Label errado. Uma sugestão:
| aria-label="Retornar ao topo" | |
| aria-label="Exibir tabela de conteúdos" |
Chamar de "tabela de conteúdo", que é uma tradução literal de "table of contents" (ToC), parece fazer mais sentido para mim do que chamar de "tópicos", mas talvez "índice" ou "sumário" fique melhor. Não sei como as plataformas costumam chamar isso em português.
| @@ -0,0 +1,17 @@ | |||
| import { Box } from '@primer/react'; | |||
There was a problem hiding this comment.
| import { Box } from '@primer/react'; | |
| import { Box } from '@/TabNewsUI'; |
| // hierarchy effect in dialog | ||
| const hierarchyEffect = (tag) => (Number(tag) - 1) * 20; |
There was a problem hiding this comment.
Aqui também não precisa do comentário, mas provavelmente poderia ter um nome melhor. Talvez getHeadingTabSize?
Rafatcb
added
front
|
@wendesongomes você conseguirá realizar as alterações que comentei na revisão? Se não conseguir, não tem problema, mas fecharei o PR para organizar o repositório e deixarei como referência no issue para quando alguém for continuar a implementação, já que possivelmente o código ou a discussão podem ser "reaproveitados". |
Mudanças realizadas
(Removi o outro PR sem querer)
Exibir tabela de conteúdos (links para os títulos) na publicação
aberto:
Resolve #1679