Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[Accessibilité] Ajout de contraste & fix de heading #55

Merged
merged 3 commits into from
May 20, 2021

Conversation

arnaudforaison
Copy link
Contributor

Petite PR afin d'améliorer l'accessibilité déjà excellente. Et de la sécurité sur les liens.

  • Ajout de rel="noreferrer" sur les liens avec target blank - Explication ici
  • Remplacement des heading (h5 notamment) avec le bon niveau
  • Meilleur gestion des contrastes

J'ai fait ces corrections grâce au rapport lighthouse de vitemadose.covidtracker.fr

@arnaudforaison arnaudforaison force-pushed the add-simple-a11y branch 2 times, most recently from 1229490 to 14de40f Compare April 13, 2021 09:43
Copy link
Collaborator

@fcamblor fcamblor left a comment

Choose a reason for hiding this comment

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

OK, je suis en phase avec toi sur le noreferrer, par contre j'ai des soucis sur :

  • Le remplacement de h5 par des h1 (il ne faut qu'un seul h1 sur la page normalement)
  • Les aspects sémantiques utilisés pour rajouter du contraste .. j'aimerais éviter d'utiliser la classe text-muted et je préfèrerais utiliser des variantes (quitte à introduire une classe dédiée dans le global.scss)

@@ -3,7 +3,7 @@
<head>
<meta charset="UTF-8">
<meta name="viewport"
content="width=device-width, user-scalable=no, initial-scale=1.0, maximum-scale=1.0, minimum-scale=1.0">
content="width=device-width, initial-scale=1.0, maximum-scale=5.0, minimum-scale=1.0">
Copy link
Collaborator

Choose a reason for hiding this comment

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

@alexandreGUY penses-tu que cela puisse poser problème de permettre le zoom ? (chez moi ça ne pose pas de problème, mais ça pose peut-être problème sur certains devices ?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alexandreGUY Est ce que tu pourrais nous dire si cela convient ou si on fait un rollback sur ce changement ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

De manière générale, il me semble que c'est une bonne pratique de retirer le tag user-scalable=no.

The user-scalable="no" parameter for the element disables browser zoom on a web page. The maximum-scale parameter limits the amount the user can zoom. Both are problematic for users with low vision who rely on browser zoom to see the contents of a web page.
https://web.dev/meta-viewport/#:~:text=The%20user%2Dscalable%3D%22no,contents%20of%20a%20web%20page

Et puis on pourra assez facilement revenir en arrière s'il y a un problème :)

<div class="col">
<h5 class="card-title">${Dates.isoToFRDatetime(this.lieu.prochain_rdv)}<small class="distance">${distance ? `- ${distance} km` : ''}</small></h5>
<h1 class="card-title h5">${Dates.isoToFRDatetime(this.lieu.prochain_rdv)}<small class="distance">${distance ? `- ${distance} km` : ''}</small></h1>
Copy link
Collaborator

Choose a reason for hiding this comment

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

@arnaudforaison Pour des raisons de SEO, il vaut mieux éviter avoir plusieurs h1 sur une page (même si ce n'est pas interdit)

cf https://fredthewebguy.me/seo/seo-and-h1-headings-according-to-google/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Je suis d'accord avec toi. Je me suis fait "berné" par Lighthouse. Mais je ne vois pas de h1 sur la page des rendez-vous. Au lieu de faire le changement comme ici, il faudrait revoir le rang des headings sur cette page ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok mais ce n'est pas ce index.html qui est utilisé ici https://vitemadose.covidtracker.fr/centres-vaccination-covid-dpt42-loire
Ma modification intervient sur cette page (uniquement il me semble)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Si c'est ce même index.html, on est sur un single page app, et c'est le index.html qui est chargée d'entrée de jeux, puis le routeur qui charge le fragment de page "variable" du site.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, mais en terme pur de rendu il n' y a pas de h1.
Il y a un h1 dans le dom mais il n'est pas rendu

Copy link
Collaborator

Choose a reason for hiding this comment

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

ouip, c'est une des questions que j'ai par rapport aux bots : je ne sais pas trop s'ils le trouvent car il est bien dans le DOM, le slot est bien "visible" (rien ne dit qu'il soit hidden) mais le slot n'est utilisé nulle part.

<div class="row">
<vmd-appointment-metadata class="mb-2" widthType="full-width" icon="vmdicon-geo-alt-fill">
<div slot="content">
<span class="fw-bold text-dark">${this.lieu.nom}</span>
<br/>
<em>${this.lieu.metadata.address}</em>
<span class="text-muted">${this.lieu.metadata.address}</span>
Copy link
Collaborator

Choose a reason for hiding this comment

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

@arnaudforaison je n'ai pas trop compris la sémantique de ce text-muted (ni la différence entre le text-muted ici et le text-dark dans la section "centre indisponible"

Sur le principe du contraste, je pense que c'est OK, mais sur la sémantique j'ai un soucis (et de plus, cela engendre une régression visuelle : le texte n'étant plus en italique)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Effectivement c'est pour le contraste. Bootstrap surcharge les em en terme de couleur. Il faudrait dans ce cas définir ou avoir des couleurs plus contrastées.
Doit on la définir dans le global ou utiliser des classes utilitaires Bootstrap ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ce que je ferais, c'est que :

  • j'essaierais de voir s'il existe déjà une couleur qui matcherait bien dans le bootstrap-variables (par exemple dans les $gray-*)
  • Je définirais plutôt des classes spécifiques à chaque composant dans un premier temps, mais utilisant cette même variable

Copy link
Collaborator

@fcamblor fcamblor left a comment

Choose a reason for hiding this comment

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

J'ai réintégré les changements récents faits sur dev

Il y a 2-3 endroits qui ont pas mal changé, je t'invite à peut-être refaire une passe sur le merge que j'ai fait (et au nouveautés qui sont apparues)

@arnaudforaison
Copy link
Contributor Author

@fcamblor Je ne peux plus resync mon fork car je suis sur Windows et les fichiers aux.json et con.json posent problème.
https://www.howtogeek.com/fyi/windows-10-still-wont-let-you-use-these-file-names-reserved-in-1974/

@fcamblor
Copy link
Collaborator

fcamblor commented May 2, 2021

@arnaudforaison sur la PR #108 il semble que @bilelz ait eu le même problème que toi sur aux.json et con.json.

cf les commits 9fa9052, 0440edb et 7a48b96 sur la PR

J'aimerais comprendre ce qui ne va pas pour résoudre le problème à la source (dans le tools/communes-import.js qui génère ces fichiers)

@arnaudforaison
Copy link
Contributor Author

En fait les fichiers avec ces noms ne fonctionnent pas sur Windows car c'est des noms de fichiers réservés.

@fcamblor fcamblor marked this pull request as draft May 4, 2021 18:26
@fcamblor
Copy link
Collaborator

fcamblor commented May 4, 2021

@arnaudforaison je viens de merger une PR (#149 ) qui fix ton soucis sur windows et la réintégrer à cette branche, tu devrais pouvoir re-checker l'état de ta PR (je l'ai re-passé en draft en attendant ton go pour une review)

@arnaudforaison arnaudforaison force-pushed the add-simple-a11y branch 2 times, most recently from a7aa333 to a42517e Compare May 7, 2021 09:59
@arnaudforaison arnaudforaison marked this pull request as ready for review May 7, 2021 10:00
@arnaudforaison
Copy link
Contributor Author

@fcamblor J'ai résolu les conflits de merge. La PR peut être relue 😉

Copy link
Collaborator

@Luwangel Luwangel left a comment

Choose a reason for hiding this comment

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

Nickel les changements, ça va faire du bien niveau SEO et accessibilité 👍

J'essaye de tester ça chez moi dans l'après-midi :)

Edit: @arnaudforaison j'ai trouvé deux nouveaux liens target blank sans noreferrer. J'ai poussé deux commits pour les corriger. Sur le reste, tout me va bien :)

@Luwangel Luwangel merged commit dc59810 into CovidTrackerFr:dev May 20, 2021
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants