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(accessibility): aria properties added to resolve accessibility a… #331

Closed
wants to merge 2 commits into from

Conversation

joaocongio
Copy link

@joaocongio joaocongio commented Oct 9, 2023

What problem is this solving?

Adicionado propriedades ARIA para resolver ajustes apontados pelo Lighthouse.
ButtonWithIcon atualmente não aceita a propriedade aria-label. Foi feito um ajuste também no app vtex.styleguide para aceitar essa propriedade.
PR Styleguide: vtex/styleguide#1447

How to test it?

Workspace

Executar o teste no Lighthouse ou Pagespeed.

Screenshots or example usage:

Antes da alteração:
minicart before

Depois da alteração (o ajuste não foi mais apontado pelo Lighthouse):
Minicart After

@joaocongio joaocongio requested a review from a team as a code owner October 9, 2023 14:37
@joaocongio joaocongio requested review from danzanzini, eduardoformiga and lucasfp13 and removed request for a team October 9, 2023 14:37
@vtex-io-ci-cd
Copy link
Contributor

vtex-io-ci-cd bot commented Oct 9, 2023

Hi! I'm VTEX IO CI/CD Bot and I'll be helping you to publish your app! 🤖

Please select which version do you want to release:

  • Patch (backwards-compatible bug fixes)

  • Minor (backwards-compatible functionality)

  • Major (incompatible API changes)

And then you just need to merge your PR when you are ready! There is no need to create a release commit/tag.

  • No thanks, I would rather do it manually 😞

@joaocongio
Copy link
Author

joaocongio commented Oct 9, 2023

Hi! I'm VTEX IO CI/CD Bot and I'll be helping you to publish your app! 🤖

Please select which version do you want to release:

  • Patch (backwards-compatible bug fixes)
  • Minor (backwards-compatible functionality)
  • Major (incompatible API changes)

And then you just need to merge your PR when you are ready! There is no need to create a release commit/tag.

  • No thanks, I would rather do it manually 😞

Copy link
Contributor

@filipewl filipewl left a comment

Choose a reason for hiding this comment

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

Boa @joaocongio! Deixei alguns comentários pra gente discutir juntos. Eu tenho a impressão que conseguimos resolver os alertas sem essas modificações feitas no styleguide. Vale tentar.

react/Minicart.tsx Outdated Show resolved Hide resolved
react/components/MinicartIconButton.tsx Outdated Show resolved Hide resolved
@@ -98,6 +98,8 @@ const MinicartIconButton: React.FC<Props> = props => {

return (
<ButtonWithIcon
id="miniCartButton"
ariaLabel="Open Minicart"
Copy link
Contributor

Choose a reason for hiding this comment

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

Entendi que passar essa prop seria permitida com essas outras modificações propostas. Porém, acredito que podemos resolver o problem inicial, do alerta de acessibilidade que botões precisam ter um nome acessível, sem precisar das modificações no repositório do styleguide!

Entendo que você estava tentando passar uma aria-label pro botão com ícone do carrinho, para que ele tivesse um nome acessível. De fato, atualmente ele não tem nenhum nome acessível:
CleanShot 2023-11-07 at 14 40 05

Mas podemos dar um nome pra ele, sem precisar passar o aria-label pra ele diretamente, e sim adicionar um aria-label no elemento do ícone passado pela prop icon, e.g.:

icon={
<span className={`${handles.minicartIconContainer} gray relative`}>

Com esse modificação aqui mesmo neste arquivo, o botão vai automaticamente herdar o nome que precisa ter:
CleanShot 2023-11-07 at 14 41 26

Faz sentido pra você? Com isso não precisaríamos dessa mudança.

Aqui só teríamos que usar aquela funcionalidade de internacionalizar o conteúdo do aria-label que indica a ação de abrir o carrinho.

Copy link
Author

Choose a reason for hiding this comment

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

Fiz as alterações usando a sua sugestão e também foi preciso adicionar a propriedade role="img", pois passou a acusar um novo erro.
image

@joaocongio
Copy link
Author

@gvc tem alguma previsão de deploy para esse ajuste? Obrigado.

@joaocore
Copy link

@gvc @filipewl esse ajuste ainda será feito deploy?

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.

5 participants