Skip to content

feat: Ajout des pictogrammes du DSFR đź’Ž V2 #417

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

Merged
merged 8 commits into from
May 28, 2025

Conversation

Maxenceee
Copy link
Contributor

@Maxenceee Maxenceee commented May 28, 2025

Prise en compte des changements demandés dans la PR #413

La version de React avait été changé par le gestionnaire de paquets en installant les nouveaux paquets, le problème a été réglé et la version est de retour à sa version initiale.

@Maxenceee Maxenceee changed the title fix : 19.1 => 18.2 feat: Ajout des pictogrammes du DSFR đź’Ž V2 May 28, 2025
@Maxenceee Maxenceee closed this May 28, 2025
@Maxenceee Maxenceee reopened this May 28, 2025
Copy link
Collaborator

@garronej garronej left a comment

Choose a reason for hiding this comment

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

Thank you for the new PR.

Please clean the diff and I'll merge and release :)

@Maxenceee
Copy link
Contributor Author

👌

@garronej garronej merged commit 2d1f631 into codegouvfr:main May 28, 2025
6 checks passed
@ddecrulle
Copy link
Collaborator

ddecrulle commented Jun 6, 2025

I believe we need to revert this PR, as it doesn't comply with the DSFR pictogram structure.

  • The <svg> must have the class fr-artwork and the attribute aria-hidden="true".
  • Each <use> element must:
    • Have a color class (e.g. fr-artwork-major)
    • Reference its path using xlink:href with a value of the form:
      ./dist/artwork/pictograms/category/pictogram-name.svg#artwork-color-id
      
  • The minor color (default: red-marianne) can be overridden using a modifier on fr-artwork, for example:
    <svg class="fr-artwork fr-artwork--green-emeraude" ... />

Currently, this implementation does not support dark mode nor color variants, both of which are required for proper DSFR theming.

@Maxenceee
Copy link
Contributor Author

Maxenceee commented Jun 6, 2025

I believe we need to revert this PR, as it doesn't comply with the DSFR pictogram structure.

* The `<svg>` must have the class `fr-artwork` and the attribute `aria-hidden="true"`.

* Each `<use>` element must:
  
  * Have a color class (e.g. `fr-artwork-major`)
  * Reference its path using `xlink:href` with a value of the form:
    ```
    ./dist/artwork/pictograms/category/pictogram-name.svg#artwork-color-id
    ```

* The minor color (default: `red-marianne`) can be overridden using a modifier on `fr-artwork`, for example:
  ```
  <svg class="fr-artwork fr-artwork--green-emeraude" ... />
  ```

Currently, this implementation does not support dark mode nor color variants, both of which are required for proper DSFR theming.

@ddecrulle Indeed I based my code on the Figma library and completely I forgot the class name and aria props.

Regarding the SVG architecture, using instead of inline paths, I thought it was more relevant in a React context, but after further consideration and considering the comments, it was a mistake.
I will propose a fix to change this and this will notably resolve the color problem reported in #424

@Maxenceee
Copy link
Contributor Author

@ddecrulle In the context of a react package that needs to be easily exported and optimized by a compiler, the approach of using static files and references to symbols in those files doesn't seem very relevant to me. This means that every project using react-dsfr will load a copy of each pictogram even without using a single one.

Based on this observation, I suggest using inline svgs, taking care to group their paths as recommended, as well as the classes and attributes required by the DSFR.

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.

3 participants