Skip to content

feat(footer): add heading level customisation on column name - #349

Open
m-maillot wants to merge 2 commits into
codegouvfr:mainfrom
m-maillot:feat/footer-heading
Open

feat(footer): add heading level customisation on column name#349
m-maillot wants to merge 2 commits into
codegouvfr:mainfrom
m-maillot:feat/footer-heading

Conversation

@m-maillot

@m-maillot m-maillot commented Dec 6, 2024

Copy link
Copy Markdown
Contributor

Pour des soucis d'optimisation SEO et d'accessibilité, il peut être intéressant de laisser la main sur le niveau des titres et la façon dont ils sont représentés dans la page.

  • Le choix du niveau est intéressant pour l'accessibilité et la logique derrière l'ordre des niveaux. DSFR utilise des h3 sur les titres des colonnes mais cela peut être incompatible avec une structure de page spécifique.

  • Le fait de pouvoir utiliser un <div role="heading" level="3"> permet d'optimiser la partie SEO. Les moteurs de recherche place ces titres à un niveau inférieur à des hX tout en ne perdant pas l'accessibilité.

il y a une issue sur un problème similaire : #345

L'idée serait de permettre de personnaliser ces informations sur différents composants et de ne pas être limité par react-dsfr. Si la lib n'est pas assez personnalisable, les contraintes SEO/Accessibilité risquent d'obliger les personnes à ne plus utiliser la lib.

Comment thread src/Footer.tsx
>;
style?: CSSProperties;
linkList?: FooterProps.LinkList.List;
linkHeadingWrapper?: {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Je l'ia placé dans une variable à part pour éviter les intégrations actuelles. Sinon on peut partir aussi sur la modification de la variable linkList sous forme de "hack" où l'on a soit un objet, soit un tableau. Si un tableau, on garde le même comportement. Si un object, on retrouve à l'intérieur les infos sur la column. On peut aussi le mettre sur le type Column mais on doit le répéter pour chaque colonne et ça serait peut trop permissif ?

Comment thread src/Footer.tsx
style?: CSSProperties;
linkList?: FooterProps.LinkList.List;
linkHeadingWrapper?: {
level: 2 | 3 | 4 | 5 | 6;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

On pourrait aussi ne pas permettre de mettre un heading ici et de pouvori mettre un simple p par exemple. A voir aussi si on ne commence pas à 3.

Comment on lines +64 to +71
"controls": { "type": null },
"description":
"Customizable list element for footers. It allows you to display a categorized list of links tailored to various needs, particularly for websites requiring a structured and accessible footer."
},
"linkHeadingWrapper": {
"controls": { "type": null },
"description":
'Allow to set a custom heading level on the column title and use a `<div role="heading" aria-level="level">` for SEO optimisation'

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

J'ai ajouté de la doc sur storybook.

@ddecrulle

ddecrulle commented Dec 17, 2024

Copy link
Copy Markdown
Collaborator

Merci pour la PR. Dans la documentation pied de page, la customisation des niveaux de titres ne semble pas possible. Avant de pouvoir merger cette PR, nous avons besoin d'avoir l'aval du SIG. En attante d'un retour GouvernementFR/dsfr#1062.

@keryanS

keryanS commented Dec 17, 2024

Copy link
Copy Markdown

Bonjour,

Une réponse à été faite dans le ticket GouvernementFR/dsfr#1062

@m-maillot

Copy link
Copy Markdown
Contributor Author

Merci pour vos retours. Pour le moment, on va optout ce composant en attendant une réponse de la part de l'équipe DSFR.

@enguerranws

Copy link
Copy Markdown
Collaborator

@m-maillot J'arrive un peu tard sur cette PR. @keryanS apporte la réponse pour moi : la proposition faite sur cette PR semble ok, avec le bémol concernant les lecteurs d'écran (= privilégier des élements de heading).

@m-maillot je fais des retours dans ce sens sur la PR.

Comment thread src/Footer.tsx
>;
style?: CSSProperties;
linkList?: FooterProps.LinkList.List;
linkHeadingWrapper?: {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
linkHeadingWrapper?: {
columnTitleAs?: {

Juste pour coller un peu plus avec un nommage qu'on retrouve souvent dans react-dsfr.

Comment thread src/Footer.tsx
{column.categoryName}
</div>
) : (
<LinkHeadingLevel

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Le fait d'utiliser des éléments de heading n'a pas d'incidence sur le rendu graphique ?

@enguerranws

Copy link
Copy Markdown
Collaborator

J'ai approuvé, avec une remarque et une question.

Signed-off-by: Julien Bouquillon <julien.bouquillon@beta.gouv.fr>
@kevbarns

Copy link
Copy Markdown
Collaborator

@m-maillot La PR est prête côté technique : vérifié sur 08d945e avec yarn build, tsc -p src, prettier --list-different et yarn test (103/103), tout passe. Le défaut level: 3 préserve le rendu actuel, donc pas de breaking change.

Elle est bloquée depuis janvier 2025 sur l'attente d'un retour du SIG. Cette réponse existe : @keryanS a répondu dans GouvernementFR/dsfr#1062 le 17/12/2024, et elle valide la démarche avec deux nuances qui touchent l'API proposée ici.

Réponse à la question ouverte de @enguerranws

Non, changer le niveau de titre n'a pas d'incidence graphique. .fr-footer__top-cat fixe font-size: 0.75rem, line-height: 1.25rem, font-weight: 700, display: block et margin: 0 0 0.75rem — soit exactement les propriétés qui distinguent h2 à h6. Aucune règle de dsfr.css ne cible hN.fr-footer__top-cat ni un sélecteur d'adjacence sur ce nœud (vérifié par grep). h2, h6 et div role="heading" rendent donc à l'identique.

À discuter

Le SIG recommande aussi <p>. « Le niveau d'en-tête des catégories peut tout à fait être ajusté en fonction de la structure d'en-tête, voire même utiliser la balise <p> pour éviter d'être trop référencé. » C'est précisément le besoin SEO exposé dans la description, et avec une balise native plutôt qu'un role. L'API gagnerait à accepter p.

role="heading" + aria-level n'est pas la voie recommandée. Toujours @keryanS : « Généralement, nous ne préconisons pas l'utilisation des attributs role="heading" et level="x" à la place de la balise dédiée. L'utilisation des balises HTML est généralement plus fiable et mieux supportée par les lecteurs d'écran. » Il l'admet pour des cas spécifiques, mais la docstring et la description Storybook le présentent aujourd'hui comme une simple « SEO optimisation ». La réserve mérite d'y figurer.

Nommage. La suggestion de @enguerranws (columnTitleAs) reste ouverte et va dans le sens du reste de la lib : titleAs existe déjà dans Card, Tile, Accordion, CallOut, Follow et Modal. linkHeadingWrapper se confond en plus avec linkListTitle, qui désigne autre chose.

Optionnel

stories/Footer.stories.tsx:69 — la nouvelle entrée utilise "controls" au lieu de "control", donc le contrôle Storybook n'est pas désactivé. La ligne 64 a la même coquille, ce sont les deux seules occurrences de tout le dossier stories/ ; les six autres entrées du fichier utilisent bien "control".

Rien à signaler côté sécurité.

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.

6 participants