Skip to content

Fix usage of div in Tile #394

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 1 commit into from
Mar 20, 2025

Conversation

benjlevesque
Copy link
Contributor

Fixes #393

@garronej
Copy link
Collaborator

Thanks :)

@garronej garronej merged commit bcfc823 into codegouvfr:main Mar 20, 2025
6 checks passed
@benjlevesque benjlevesque deleted the fix/tile/replace-p-by-div branch March 20, 2025 22:49
@m-maillot
Copy link
Contributor

m-maillot commented Jun 20, 2025

This PR introduces breaking changes. Instead of using a <p> tag for the detail, you’re now using a <div>. However, the official documentation uses a <p> tag: https://storybook.systeme-de-design.gouv.fr/?path=/docs/tile–docs#download%20button%20story

Is this a mistake, or is the change to a <div> intentional and permanent?

@benjlevesque @garronej

@benjlevesque
Copy link
Contributor Author

Usage of p means you cannot use anything else than text in the children, which is quite limiting.

Can you give an example on how does it breaks usage for you? Sorry if it does, I didn't see a visual difference.

Perhaps p should be used if the children is of type string?

@m-maillot
Copy link
Contributor

m-maillot commented Jun 20, 2025

I use it as a download tile. In the detail, I set this text:

detail={`Format ${extension} - ${filesize}Ko`}

So now, the generated HTML is:

<div
  class="fr-tile__detail"
>
  Format PDF - 3200Ko
</div>

Instead of:

<p
  class="fr-tile__detail"
>
  Format PDF - 3200Ko
</p>

Which is incorrect in terms of HTML structure and accessibility.

Moreover, as @garronej mentioned in the issue:

Thanks, the newer reference template actually uses p instead of divs.

systeme-de-design.gouv.fr/composants-et-modeles/composants/tuile

This is the only argument that matters. We implement the official recomendation, we don't take decision here.

This PR goes in the opposite direction.

It’s not a big issue — we can adapt our code. I just want to avoid having the same kind of inconsistency again in the future regarding the official documentation.

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.

Cannot use div in Tile's content
3 participants