-
Notifications
You must be signed in to change notification settings - Fork 9
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
fix(header): improve mobile menu accessibility #157
base: main
Are you sure you want to change the base?
Conversation
Deploy preview for portal-ascepa ready! ✅ Preview Built with commit db65d3d. |
…chexpertspro/portal-ascepa into fix/mobile-menu-accessibility
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.
Fala @patzsl! Testei o menu pelo Deploy preview e por algum motivo o botão do menu não está aparecendo no mobile
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.
Code Review concluído com Sucesso. OK.
|
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.
Agora ficou bem massa! Só um ponto que poderia ser aprimorado é deixar o texto "Menu" clickável, como se ele fizesse parte do botão. Um exemplo legal para se basear é o menu mobile da W3C:
https://www.w3.org/
fill="none" | ||
xmlns="http://www.w3.org/2000/svg" | ||
<div id="menu-toggle-container"> | ||
<h2 id="menu-title" class="mr-1rem">Menu Principal</h2> |
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.
- Caso seja necessário usar uma tag, recomendaria usar o span, pois o h2 deixou a fonte muito grande.
- Uma outra sugestão seria colocar o texto, com ou sem tag(conforme necessidade), dentro da tag button, pois assim não precisaria colocar a tag div envolvendo todo mundo. Quanto menos tag melhor : D
Excelente ponto @Flamilani! Acho que seria interessante fazer a correção nessa PR mesmo. |
Descrição
Adiciona titulo e acessibilidade no menu mobile, como descrito na task de a11y #156
Tipo da alteração
fix: Correção de acessibilidade
Checklist