Repository navigation
Conversation
…les identifiants uniques ## Pourquoi les changements ont été faits : - Le champ était décrit par l’indice en toutes circonstances : le message d’erreur, pourtant identifié, n’était jamais restitué au lecteur d’écran, et l’indice l’était même vide - Les quatre identifiants étaient codés en dur : deux lettres d’information sur une même page produisaient des doublons, et le libellé de la seconde désignait le champ de la première - Le libellé est masqué visuellement dans ce composant, donc une association rompue ne laisse plus rien à une personne qui navigue au lecteur d’écran ## Quelles modifications ont été apportées : - Prop `id` optionnelle, engendrée par `useRandomId` comme dans `DsfrInput`, dont sont dérivés les identifiants du bouton, de l’indice et du message d’erreur - `aria-describedby` calculé : l’erreur si elle existe, sinon l’indice s’il est renseigné - `aria-invalid` et `fr-input--error` sur le champ en erreur, indice rendu seulement s’il existe - Cinq tests ajoutés et la nouvelle prop documentée closes #1395
…me seul ## Pourquoi les changements ont été faits : - Les pictogrammes du DSFR ne sont affichables qu’à travers `DsfrTile` ou `DsfrRadioButton`, alors qu’ils servent aussi seuls dans une page - Une balise `use` ne sait pas référencer un fragment dans une data URI, or un bundler transforme en data URI les pictogrammes les plus légers : un rendu naïf par `use` en fait disparaître une partie sans aucune erreur - Deux pictogrammes en ligne dans une même page partagent les identifiants de leurs calques ## Quelles modifications ont été apportées : - Nouveau composant `DsfrPictogram`, avec ses types, ses tests, sa story, sa documentation et sa démo - Rendu choisi selon la source : image, SVG en ligne nettoyé, image de repli, ou référence aux trois calques `artwork-*` - Identifiants des calques suffixés pour garder indépendants deux pictogrammes d’une même page - Composant ajouté à la liste des composants, à la barre latérale de la documentation et au guide des pictogrammes officiels closes #1036
## Pourquoi les changements ont été faits : - `DsfrTile` et `DsfrRadioButton` portaient chacun les quatre branches de rendu d’un pictogramme, dupliquées à l’identique - L’issue #1293 avait mutualisé le décodage des data URI dans `svg-data-uri-utils`, mais pas le rendu qui l’entoure - Le rendu des pictogrammes n’était couvert par aucun test dans ces deux composants ## Quelles modifications ont été apportées : - Rendu du pictogramme délégué à `DsfrPictogram` dans les deux composants - Suppression des expressions calculées, des constantes et du style devenus inutiles - Ajout d’un test de rendu du pictogramme dans chacun des deux composants
…-ids-1395 Corrige les identifiants et le message d’erreur de la lettre d’information
Ajoute le composant DsfrPictogram pour afficher un pictogramme seul
✅ Deploy Preview for vue-dsfr ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for demo-vue-dsfr ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
DsfrPictogram does not apply svgAttrs when rendering an inlined data-URI SVG, creating inconsistent behavior for a common input type and contradicting the prop’s documented intent.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR introduces a new DsfrPictogram component to centralize DSFR pictogram rendering (sprite URL vs bundled data-URI), and refactors existing components to rely on it while improving DsfrNewsLetter’s accessibility IDs.
Changes:
- Added
DsfrPictogramcomponent (+ types, tests, Storybook story, and VitePress docs) and exported it publicly. - Refactored
DsfrTileandDsfrRadioButtonrich pictogram rendering to useDsfrPictogram(and added/updated tests accordingly). - Improved
DsfrNewsLetterby generating per-instance IDs and wiringfor/aria-describedby/ error styling, with new tests and doc updates.
File summaries
| File | Description |
|---|---|
| src/components/index.ts | Exports DsfrPictogram component + types from the public components barrel. |
| src/components/DsfrTile/DsfrTile.vue | Replaces inline pictogram rendering logic with DsfrPictogram. |
| src/components/DsfrTile/DsfrTile.spec.ts | Adds coverage to ensure pictogram sprite layers are rendered in tiles. |
| src/components/DsfrRadioButton/DsfrRadioButton.vue | Replaces rich radio pictogram rendering logic with DsfrPictogram. |
| src/components/DsfrRadioButton/DsfrRadioButton.spec.ts | Adds coverage for pictogram rendering + title in rich radio buttons. |
| src/components/DsfrPictogram/DsfrPictogram.vue | New shared pictogram component handling image, sprite SVG, and data-URI inline SVG cases. |
| src/components/DsfrPictogram/DsfrPictogram.types.ts | Defines DsfrPictogram public props. |
| src/components/DsfrPictogram/DsfrPictogram.stories.ts | Adds Storybook stories for standard and resized pictograms. |
| src/components/DsfrPictogram/DsfrPictogram.spec.ts | Adds unit tests for all rendering branches and ID-suffix behavior. |
| src/components/DsfrPictogram/DsfrPictogram.md | Adds component documentation and demo integration. |
| src/components/DsfrPictogram/docs-demo/DsfrPictogramDemo.vue | Adds demo used by VitePress docs page. |
| src/components/DsfrFollow/DsfrNewsLetter.vue | Generates unique IDs and improves label/input association + error/hint accessibility wiring. |
| src/components/DsfrFollow/DsfrNewsLetter.spec.ts | Adds tests validating label association, described-by behavior, and ID uniqueness across instances. |
| src/components/DsfrFollow/DsfrNewsLetter.md | Documents the new id prop. |
| src/components/DsfrFollow/DsfrFollow.types.ts | Adds id?: string to DsfrNewsLetterProps. |
| docs/guide/pictogrammes.md | References DsfrPictogram as the recommended way to render DSFR pictograms. |
| docs/composants.md | Adds DsfrPictogram to the components list. |
| .vitepress/config.ts | Adds DsfrPictogram to the VitePress sidebar/nav config. |
Review details
- Files reviewed: 18/18 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+18
to
+41
| const dataImageRegex = /^data:image\/svg\+xml(?:;[^,]*)?,/i | ||
| const defaultSvgAttrs = { viewBox: '0 0 80 80', width: '80px', height: '80px' } | ||
|
|
||
| const inlineSvgIdSuffix = useRandomId('pictogram', 'artwork').replace(/[^\w-]/g, '_') | ||
| const svgDataUriComputed = computed(() => !!props.svgPath?.match(dataImageRegex)) | ||
| const svgDataUriMarkupComputed = computed(() => ( | ||
| sanitizeInlineSvgMarkupFromDataUri(props.svgPath, inlineSvgIdSuffix) | ||
| )) | ||
| </script> | ||
|
|
||
| <template> | ||
| <img | ||
| v-if="imgSrc" | ||
| :src="imgSrc" | ||
| class="fr-artwork" | ||
| alt="" | ||
| :title="title" | ||
| > | ||
| <span | ||
| v-else-if="svgDataUriMarkupComputed" | ||
| aria-hidden="true" | ||
| class="fr-artwork fr-artwork--inline" | ||
| v-html="svgDataUriMarkupComputed" | ||
| /> |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 477e770570
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+36
to
+40
| <span | ||
| v-else-if="svgDataUriMarkupComputed" | ||
| aria-hidden="true" | ||
| class="fr-artwork fr-artwork--inline" | ||
| v-html="svgDataUriMarkupComputed" |
There was a problem hiding this comment.
When svgPath is a valid data URI—as occurs for pictograms inlined by a bundler—this branch renders the sanitized markup without applying svgAttrs. Consequently documented calls such as the resized examples in DsfrPictogram.md leave the pictogram at its original/default dimensions, while the same attributes work for URL-based SVGs; apply the attributes to the inlined SVG or an equivalent wrapper style.
Useful? React with 👍 / 👎.
Comment on lines
+36
to
+40
| <span | ||
| v-else-if="svgDataUriMarkupComputed" | ||
| aria-hidden="true" | ||
| class="fr-artwork fr-artwork--inline" | ||
| v-html="svgDataUriMarkupComputed" |
There was a problem hiding this comment.
When a consumer supplies title together with a valid data-URI svgPath, this branch neither inserts the title into the sanitized markup nor exposes it on the wrapper, which is also unconditionally hidden from assistive technology. This silently drops the documented title for the bundler-import scenario the component is designed to support, so informative pictograms provide neither the expected tooltip nor an accessible name.
Useful? React with 👍 / 👎.
…inlinés en data URI ## Pourquoi les changements ont été faits : - Quand svgPath est une data URI (cas d’un pictogramme importé par un bundler), le SVG est rendu via v-html, ce qui empêche Vue d’appliquer svgAttrs ou un titre comme sur les autres branches de rendu (img, SVG sprite) - Les exemples de redimensionnement documentés dans DsfrPictogram.md et la story PictogrammesDeTaillesDifferentes n’avaient donc aucun effet, et le titre ne produisait plus d’info-bulle au survol pour ce cas d’usage pourtant courant ## Quelles modifications ont été apportées : - Ajout d’un style calculé (inlineSvgWrapperStyle) dérivé de svgAttrs, appliqué à l’élément englobant du SVG inliné pour piloter sa taille via le CSS existant - Ajout de l’attribut title sur ce même élément englobant, pour retrouver l’info-bulle déjà présente sur les branches img et SVG sprite - Ajout de tests unitaires couvrant ces deux régressions dans DsfrPictogram.spec.ts closes #1405
…tion des reverts ## Pourquoi les changements ont été faits : - Le CHANGELOG n’était pas généré ni commité automatiquement après une release semantic-release - La configuration de release-notes-generator omettait le type revert, ce qui faisait disparaître ces commits du CHANGELOG alors qu’ils déclenchent bien une release (règle par défaut du commit-analyzer) ## Quelles modifications ont été apportées : - Ajout de la dépendance @semantic-release/git et du plugin correspondant pour committer CHANGELOG.md et package.json après chaque release - Ajout de la section ⏪ Annulations pour le type revert dans la configuration de release-notes-generator
## Pourquoi : - svgAttrs désigne un ensemble d'attributs, « le reste de l'attribut » était incohérent avec le pluriel - Relevé en commentaire de revue sur la PR #1406 ## Quoi : - Remplace « le reste de l'attribut » par « le reste des attributs » dans le commentaire de inlineSvgWrapperStyle
…-attrs-1405 fix(DsfrPictogram): 🐛 applique svgAttrs et le titre aux pictogrammes inlinés en data URI
|
🎉 This PR is included in version 8.21.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.