entrouvert
/
publik-base-theme
16
1
Fork 1
Code Issues Pull Requests 7 Releases Wiki Activity

Labels Milestones

a11y: toggle nav button label when opened (#75679) #206

Merged
fpeters merged 1 commits from wip/75679-a11y-open-menu-label into main 2 months ago
Conversation 0 Commits 1 Files Changed 1
fpeters commented 2 months ago
Owner

Ça ne passe plus par le code générique "togglable" (de combo.public.js), ça supprime également la classe sur le div#nav, j'ai souvenir de conversations récentes qui parlaient déjà de ça mais je n'ai pas retrouvé de ticket/PR associé.

JS fait sans jquery pour suivre ce qu'on disait à eodays/front/lyon, c'est tout petit mais s'il y a des commentaires de forme pour que ça serve de modèle correct, go for it.

Ça ne passe plus par le code générique "togglable" (de combo.public.js), ça supprime également la classe sur le div#nav, j'ai souvenir de conversations récentes qui parlaient déjà de ça mais je n'ai pas retrouvé de ticket/PR associé. JS fait sans jquery pour suivre ce qu'on disait à eodays/front/lyon, c'est tout petit mais s'il y a des commentaires de forme pour que ça serve de modèle correct, go for it.
fpeters added 1 commit 2 months ago
gitea/publik-base-theme/pipeline/head This commit looks good Details
41db03a375 a11y: toggle nav button label when opened (#75679)
tjund requested review from tjund 2 months ago
csechet approved these changes 2 months ago
csechet left a comment
Owner

En fait c'est l'inverse : on disait de poser la classe "toggled" sur la nav, et pas sur le bouton, pour éviter d'avoir à devoir utiliser des sélecteurs du type #nav-button.toggled + .menu, qui oblige d'avoir le menu juste après le bouton. Ça permettrait de pouvoir modifier le HTML du menu burger et y ajouter des choses (toplinks, recherche...), ce qui est souvent demandé par les clients.

Mais en l'état ça fixe le ticket associé. On pourra gérer cette histoire le jour où on aura un togglable générique dans Gadjo (si c'est toujours le plan d'avoir le JS de Combo dans Gadjo).

En fait c'est l'inverse : on disait de poser la classe "toggled" sur la nav, et pas sur le bouton, pour éviter d'avoir à devoir utiliser des sélecteurs du type #nav-button.toggled + .menu, qui oblige d'avoir le menu juste après le bouton. Ça permettrait de pouvoir modifier le HTML du menu burger et y ajouter des choses (toplinks, recherche...), ce qui est souvent demandé par les clients. Mais en l'état ça fixe le ticket associé. On pourra gérer cette histoire le jour où on aura un togglable générique dans Gadjo (si c'est toujours le plan d'avoir le JS de Combo dans Gadjo).
tjund requested changes 2 months ago
templates/includes/nav.html Outdated
@ -24,0 +26,4 @@
<script>
(function() {
const nav_button = document.getElementById('nav-button');
const nav_button_label = nav_button.getElementsByClassName('sr-only')[0];
tjund commented 2 months ago
Owner

Cibler un element via une class utilitaire n'est pas une bonne idée.

Ensuite, l'attibut aria-label remplace le contenu, c'est à cet attribut qu'il faut faire changer le contenu.

Cibler un element via une class utilitaire n'est pas une bonne idée. Ensuite, l'attibut aria-label remplace le contenu, c'est à cet attribut qu'il faut faire changer le contenu.
fpeters commented 2 months ago
Poster
Owner

Ensuite, l'attibut aria-label remplace le contenu, c'est à cet attribut qu'il faut faire changer le contenu.

L'audit notait de manière confuse "L'attribut aria-label est toujours "Ouvrir le menu" même quand il est ouvert", j'ai donc imaginé que ça lisait "Ouvrir le menu" et pas le "Menu" de aria-label, et c'est donc ça que j'ai changé.

Je fais une synthèse dans le patch modifié, avec désormais un seul libellé :

-      <button id="nav-button" class="gru-nav-button togglable" aria-label="Menu">
-        <span class="sr-only">Ouvrir le menu</span>
+      <button id="nav-button" class="gru-nav-button" aria-labelledby="nav-button--label">
+        <span class="sr-only" id="nav-button--label">Ouvrir le menu</span>

(et ça donne ainsi un id qui peut être ciblé, plut

> Ensuite, l'attibut aria-label remplace le contenu, c'est à cet attribut qu'il faut faire changer le contenu. L'audit notait de manière confuse "L'attribut aria-label est toujours "Ouvrir le menu" même quand il est ouvert", j'ai donc imaginé que ça lisait "Ouvrir le menu" et pas le "Menu" de aria-label, et c'est donc ça que j'ai changé. Je fais une synthèse dans le patch modifié, avec désormais un seul libellé : ``` - <button id="nav-button" class="gru-nav-button togglable" aria-label="Menu"> - <span class="sr-only">Ouvrir le menu</span> + <button id="nav-button" class="gru-nav-button" aria-labelledby="nav-button--label"> + <span class="sr-only" id="nav-button--label">Ouvrir le menu</span> ``` (et ça donne ainsi un id qui peut être ciblé, plut
tjund commented 2 months ago
Owner

Top. Parfait pour moi comme ça.

Top. Parfait pour moi comme ça.
fpeters force-pushed wip/75679-a11y-open-menu-label from 41db03a375 to 23dbec3ac7 2 months ago
fpeters requested review from tjund 2 months ago
tjund approved these changes 2 months ago
fpeters merged commit 460825aa3b into main 2 months ago
fpeters deleted branch wip/75679-a11y-open-menu-label 2 months ago

Reviewers

csechet approved these changes 2 months ago
tjund approved these changes 2 months ago
gitea/publik-base-theme/pipeline/head This commit looks good
Details
The pull request has been merged as 460825aa3b.
You can also view command line instructions.

Step 1:

From your project repository, check out a new branch and test the changes.
git checkout -b wip/75679-a11y-open-menu-label main
git pull origin wip/75679-a11y-open-menu-label

Step 2:

Merge the changes and update on Gitea.
git checkout main
git merge --no-ff wip/75679-a11y-open-menu-label
git push origin main
Sign in to join this conversation.
Reviewers
Request review
No reviewers
csechet
tjund
Labels
Apply labels
Clear labels
No items
No Label
Milestone
Set milestone
Clear milestone
No items
No Milestone
Assignees
Assign users
Clear assignees
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: entrouvert/publik-base-theme#206
Write Preview
Loading…
Cancel
Save
There is no content yet.
Delete Branch 'wip/75679-a11y-open-menu-label'

Deleting a branch is permanent. It CANNOT be undone. Continue?

No
Yes