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

feat: navbar mobile #1279

Closed
wants to merge 21 commits into from
Closed

Conversation

Ryannnkl
Copy link
Contributor

@Ryannnkl Ryannnkl commented Mar 1, 2023

Gravacao.de.tela.de.28-02-2023.22.56.06.webm

@vercel
Copy link

vercel bot commented Mar 1, 2023

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

A member of the Team first needs to authorize it.

@Ryannnkl Ryannnkl changed the title feat: Menu mobile feat: navbar mobile Mar 1, 2023
@Ryannnkl
Copy link
Contributor Author

Ryannnkl commented Mar 1, 2023

image

@Ryannnkl
Copy link
Contributor Author

Ryannnkl commented Mar 1, 2023

esse componente de Diálogo que to usando para esse menu tem um problema com o botão de fechar que já é embutido no componente, fica dessa forma se eu aumentar o tamanho da barra para combinar com a barra padrão

Gravacao.de.tela.de.01-03-2023.17.22.41.webm

tamanho padrão e sem barra

tamanho padrão sem barra
image image

gostaria de opiniões sobre o que fazer, se talvez trocar o componente de Diálogo pelo mesmo componente que ta sendo usado no botão do perfil, ou algo parecido

@aprendendofelipe
Copy link
Collaborator

Boa @Ryannnkl, essa implementação é requisito para diversas outras!

O que acha de usar ActionMenu no lugar de Dialog?

@Ryannnkl
Copy link
Contributor Author

Ryannnkl commented Mar 1, 2023

video1.webm

ps:
o action menu não deixa muito bem a gente customizar o icone, caso a gente tente colar apenas o icone sem esse background branco fica assim:

image image

@aprendendofelipe
Copy link
Collaborator

o action menu não deixa muito bem a gente customizar o icone, caso a gente tente colar apenas o icone sem esse background branco fica assim:

Será que não basta adicionar variant="invisible" no IconButton?

@Ryannnkl
Copy link
Contributor Author

Ryannnkl commented Mar 1, 2023

O exemplo que coloquei abaixo com o ícone azul foi porque eu coloquei o variant="invisible"

e usar um botão externo como nessa parte da documentação o estado de "open" não funciona por alguma razão.

tentarei de novo amanhã, mas não ficou feio com o botão preenchido, talvez no print não tenha ficado tão charmoso kk

@Ryannnkl
Copy link
Contributor Author

Ryannnkl commented Mar 2, 2023

Talvez se quando estiver logado, juntar tudo em apenas um lado:

image image

@Ryannnkl Ryannnkl marked this pull request as ready for review March 2, 2023 23:22
@Ryannnkl
Copy link
Contributor Author

Ryannnkl commented Mar 2, 2023

resultado:

image

@vercel
Copy link

vercel bot commented Mar 6, 2023

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

Name Status Preview Comments Updated
tabnews ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Mar 18, 2023 at 1:41PM (UTC)

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.

Olá @Ryannnkl, fiz uns comentários no código.

Talvez se quando estiver logado, juntar tudo em apenas um lado:

Eu acho interessante. Inclusive é o que o GitHub faz. E deve ficar mais interessante a disposição do que sobrou no Header.

Vamos aguardar outras opiniões sobre o resultado?

pages/interface/components/Header/index.js Outdated Show resolved Hide resolved
pages/interface/components/Header/index.js Outdated Show resolved Hide resolved
@aprendendofelipe
Copy link
Collaborator

Turma, o que vocês acharam desse resultado que pode ser visto no link e na imagem abaixo?

https://tabnews-git-fork-ryannnkl-menu-mobile-tabnews.vercel.app/

image

Eu acho que precisa de ajustes.

Poderia unificar os menus do lado esquerdo, deixar o logo centralizado e variar o que aparece na direita de acordo com o usuário logado ou não.

Sem usuário logado poderia ficar com um botão "Entrar" na direita.

Já logado eu acho que poderia mostrar os saldos ou um botão de publicar.

Minhas sugestões:

image

@ermesonqueiroz
Copy link
Contributor

image

Esse exemplo ficou ótimo, os únicos ponto que vejo são: a presença do botão "Publicar novo conteúdo" em 2 lugares (na barra de navegação e no menu) e a identificação dos TabCoins e TabCash (mas isso pode ser discutido em outra issue)

@filipedeschamps
Copy link
Owner

Ta tudo muito massa 😍

E sobre a dúvida:

image

Eu acabo olhando as TabCoins que tenho muito mais que publico conteúdos root, então eu deixaria elas visíveis ao invés do +

@Ryannnkl
Copy link
Contributor Author

Ryannnkl commented Mar 15, 2023

@aprendendofelipe

sobre o selectionVariant o estilo dele deixa um espaço em branco antes do texto nos itens não selecionados:
image

vou enviar as atualizações sem o selectionVariant e esperar por alguma ideia

@aprendendofelipe
Copy link
Collaborator

sobre o selectionVariant o estilo dele deixa um espaço em branco antes do texto nos itens não selecionados:

Fica melhor usando NavList, né?

Eu acho que "TabNews", "Navegar" e "Usuário" não precisam estar no menu.

Olhando sua implementação, vi que usou o createRef. É mesmo necessário?

@Ryannnkl
Copy link
Contributor Author

Ryannnkl commented Mar 15, 2023

a referência em questão é usada para o botão do menu, seguindo essa documentação: With External Anchor
, caso seja um problema eu posso deixar um botão padrão do ActionMenu dessa forma:
image

o variant="invisible" não funciona muito bem nesse componente de IconButton do primer. por isso que utilizei uma ref para um botão externo que eu pudesse controlar de forma íntegra.

a respeito do Navegar e Usuário, só é uma separação por grupos não é um botão de ação, vou atualizar e deixar sem no próximo commit

@Ryannnkl
Copy link
Contributor Author

assim agora:
image

@aprendendofelipe
Copy link
Collaborator

a referência em questão é usada para o botão do menu, seguindo essa documentação: With External Anchor , caso seja um problema eu posso deixar um botão padrão do ActionMenu dessa forma:

Não é problema não. Era só uma dúvida. Acho que fica bem melhor com o botão anterior 😉

image

Sobre o design, na minha opinião é quase isso que está na sua última mensagem. Mas eu voltaria o botão anterior e centralizaria o logotipo do TabNews.

Um ponto extra é que eu criei o componente NavItem e ele já está disponível na branch main. Ele pode ser utilizado no lugar do ActionList.Item. Caso queira utilizar, fica como na imagem abaixo, e acho que ficou legal. 👍

image

O que acham?

@aprendendofelipe
Copy link
Collaborator

aprendendofelipe commented Mar 18, 2023

Boa @Ryannnkl. 💪

Acho que faltam "poucos" ajustes, mas caso não vá ter tempo de realizar, ou tenha alguma dificuldade, faz um squash dos commits desse PR pra que eu possa aproveitar esse commit unificado em um novo PR e fazer os ajustes finais. 🤝

Botão de menu

  • Padronizar cores: border, hover etc. E mesmo que ainda não tenhamos dark-mode, de preferência devem ter o mesmo comportamento do restante do header ao selecionar outro tema.

  • Parace que não precisa mesmo do createRef para usar qualquer botão que queira. Basta colocar dentro do ActionMenu.Anchor.

  • [Sugestão] Já que criou esse novo botão, usar ele na direita também. Nesse caso, acho que pode trocar o ícone do username para PersonIcon.

Arrumar apenas em telas pequenas

  • Centralizar logotipo considerando o header todo. Utilizar grid pode ser uma opção;

  • Trocar links para "Login" e "Cadastrar" por "Entrar" (direcionando para a tela de login);

  • Arrumar o padding do header para alinhar os itens das extremidades com o restante do conteúdo da página;

  • Mostrar qual é o usuário logado (faltou isso na minha sugestão de design);

  • Mostrar se estamos nos relevantes ou recentes mesmo quando não estiver na primeira página. Sugestão abaixo:

const isRelevantPath = pathname === '/' || pathname.startsWith('/pagina');
const isRecentPath = pathname.startsWith('/recentes');

[Editei acima] Testei e não precisar passar 'page' como no exemplo do Primer, então é melhor passar true.

Então usa assim:

<NavItem href={`/`} aria-current={isRelevantPath}>

E dá pra aproveitar essas novas variáveis nos HeaderLink também.

Acho que é "só" isso 😁

@Ryannnkl
Copy link
Contributor Author

quando se refere a alinhar o padding com o restante do o conteúdo pensa nesse espaçamento?

image

a respeito de também colocar esse ícone do outro lado, se refere a isso?

image

e por fim a respeito de mostrar o usuário logado é algo que me deixou em dúvida:

image

se colocar dessa forma fica duplicado no meus conteúdos e ao Clicar no botão com nome do usuário.

@aprendendofelipe
Copy link
Collaborator

quando se refere a alinhar o padding com o restante do o conteúdo pensa nesse espaçamento?

image

Isso mesmo @Ryannnkl, centralizado com relação ao header todo, independentemente dos outros itens 👍


a respeito de também colocar esse ícone do outro lado, se refere a isso?

image

Isso mesmo. O que acham? Dessa imagem eu só aumentaria o espaçamento entre o saldo de TabCash e o botão do menu para que fique igual ao que há entre os outros itens do header 🤝


e por fim a respeito de mostrar o usuário logado é algo que me deixou em dúvida:

image

se colocar dessa forma fica duplicado no meus conteúdos e ao Clicar no botão com nome do usuário.

Ficar dois itens que direcionam para a mesma página não é nenhum problema, Mas só habilite o aria-current em um deles. O outro fica sempre como false 👍

@Ryannnkl
Copy link
Contributor Author

não estou conseguindo entender as tarefas que me foram passadas com o area-current, então vou enviar o último commit e deixar que alguém mais capacitado consiga fazer, pois já está demorando bastante com essa PR aberta, lamento o atraso.

@aprendendofelipe
Copy link
Collaborator

não estou conseguindo entender as tarefas que me foram passadas com o area-current, então vou enviar o último commit e deixar que alguém mais capacitado consiga fazer, pois já está demorando bastante com essa PR aberta, lamento o atraso.

Fala @Ryannnkl, vou explicar abaixo só para tentar sanar sua dúvida, mas se não for seguir adiante com o PR, não tem problema, pois sua ajuda já foi muito importante para estudarmos as alternativas dessa implementação. 🤗

O aria-current é o indicador de qual dos links do menu é referente a página que estamos. Por isso apenas um desses elementos pode receber valor diferente de false por vez. É um item de acessibilidade.

O componente NavList.Item do Primer já tenta setar isso por conta própria e ele também faz a estilização com base nesse parâmetro (aquela barra azul), mas com a navegação do Next.js ele precisa de uma modificação, por isso criei o NavItem que usa o componente do Primer, mas entende o caminho que estamos, mesmo navegando em SPA.

Mas ainda assim é preciso, em alguns casos, passar a propriedade aria-current para o componente, pois ele não vai entender que estamos vendo os conteúdos recentes quando não estivermos na primeira página dos recentes, pois /recentes é diferente de /recentes/pagina/2. E aqui a razão que eu falei para passar true e não 'page', pois assim estaremos indicando para a tecnologia assistiva que aquele link não leva exatamente para a página atual, mas apenas que é a mesma categoria.

E o outro caso que devemos passar a propriedade eu acho que você entendeu, pois vi que já fez. É quando tiver mais de um link que leva para a mesma página, como é o caso do nome do usuário e dos "meus conteúdos". Nesse caso é melhor passar false para um deles e deixar apenas o outro identificar de acordo com a página atual.

@aprendendofelipe
Copy link
Collaborator

Vou fechar esse PR por inatividade, mas marquei com a tag repescar, pois o que foi discutido e produzido aqui tem muito valor!

Muito obrigado @Ryannnkl! 💪

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants