search

codegouvfr / react-dsfr

🇫🇷 The French Government Design system React toolkit
https://react-dsfr.codegouv.studio
MIT License
416 stars 55 forks source link

Pas d'affichage quand checkbox sans label #281

Closed totakoko closed 3 months ago

totakoko commented 3 months ago

Hello ! :wave:

Juste pour info, sur FCU, on utilise un tableau avec une colonne qui ne contient que des checkbox sans label (label=''). Depuis peu avec https://github.com/codegouvfr/react-dsfr/pull/275, l'élément label disparait mais également le style de la checkbox, car le DSFR utilise le label pour placer sa checkbox custom au dessus de l'input.

.fr-checkbox-group input[type=checkbox] + label::before {
//...
}

=> On peut s'en sortir en mettant un espace en label. ou sinon il faudra qu'on copie le style et qu'on implémente nous-mêmes une checkbox sans label.

En l'état, ça veut dire que si pas de label, la checkbox react-dsfr ne s'affichera jamais, donc elle n'est pas vraiment utilisable. :shrug:

garronej commented 3 months ago

Hello @totakoko,

Merci pour le rapport @ddecrulle, je te laisse voir ça?

ddecrulle commented 3 months ago

Bonjour,

Dans ce cas je pense qu'il ne faut pas utiliser la Checkbox de la librairie. En effet elle est est conçu pour afficher plusieurs <input type="checkbox" />, entourés d'un <fieldset /> etc...

Le cas que tu mentionnes a d'ailleurs été pensé par le dsfr. Tu verras qu'il y a bien un label (pour l'accessibilité) qui n'est simplement pas affiché. Tu peux retrouver ça là : Tableau avec lignes sélectionnables

je vais regarder quand même, car ça reste problématique que sans label, la checkbox ne soit pas affichée

totakoko commented 3 months ago

J'avais vu ce nouveau tableau mais il n'est pas (encore) implémenté dans la lib react-dsfr, donc c'est pas utilisable de notre côté. On a préféré utiliser la Data Grid MUI pour avoir le tri / pagination auto. Mais donc on perd la finesse des classes DSFR.

Aussi en regardant comment ils ont fait, je suis pas méga convaincu de la règle CSS spécifique qui passe le label en transparent. Ça ne fonctionnera que pour une cellule fixed (qui est sticky), cad juste la sélection de ligne. Or chez nous, ce n'est pas une sélection de ligne mais juste un état d'email envoyé ou non. image

En tout cas avec un espace c'est OK pour nous, ça pourrait juste casser pour d'autres personnes.

garronej commented 3 months ago

@totakoko Oui, je pense que c’est une bonne idée d’utiliser Data Grid MUI. Merci @ddecrulle pour le partage du nouveau tableau. Par contre, il est complètement headless, donc il y a beaucoup de plomberie à mettre en place pour le faire fonctionner. Il faut, par exemple, implémenter soi-même la pagination, le tri, etc. C’est un problème qui a déjà été résolu mille et une fois. Réimplémenter ce type de logique plutôt que de se concentrer sur la logique métier n’est pas justifié à mon avis. Après, bien sûr, on pourrait implémenter cette logique pour vous côté react-dsfr, mais il y a de grandes chances que ce que nous produirions soit moins flexible que MUI Datagrid. Compte tenu du fait que la data grid de MUI est déjà adaptée au DSFR automatiquement, je ne sais pas si c’est très rentable d’investir du temps dans l’implémentation de ce tableau. À mon avis, mieux vaut conseiller aux gens d’utiliser MUI pour cela.

D’ailleurs, @totakoko, utilisez-vous le MUI provider de react-dsfr ?

totakoko commented 3 months ago

Oui le composant DSFR est fait juste pour un tableau statique très simple.

Pour le style automatique de la Data Grid MUI, et bien je suis pas très sûr, comment je peux vérifier ça ? Car on a dû faire des styles un peu custom...

ddecrulle commented 3 months ago

Pour vérifier ça il faut regarder si tu as le provider <MuiDsfrThemeProvider> dans ta codebase. Je ne l'ai pas trouvé. Le provider ajoute du style un peu comme vous l'avez fait. C'est pas impossible qu'il manque quelques styles, si dans le futur tu l'utilises et vois des oublies n'hésites pas à nous faire signe.

totakoko commented 3 months ago

Dans ce cas oui alors je suppose qu'on a ces styles. C'est sur notre (grosse) branche migrate_to_codegouvfr_react_dsfr qu'on espère merger cette semaine...

Là on essaie de migrer de lib en restant le plus iso-fonctionnel possible, sachant que FCU contient plein d'endroits pas super carrés niveau DSFR, mais ça viendra... :)

totakoko commented 3 months ago

Merci à vous 2 pour votre super réactivité ! :zap: :muscle:

garronej commented 3 months ago

@totakoko Yes c'est bon vous utilisez le MUI DSFR provider: https://github.com/betagouv/france-chaleur-urbaine/blob/729d2d20ddef3dfae80f1a0b7f17222fbaca053e/src/MuiDsfrThemeProvider.tsx#L12-L19!

Vous l'augmentez même donc je suis content, je pense que vous êtes les seul a avoir compris que c'etais possible de faire ça ^^.

Par contre j'ai ajouter les style custom pour la data grid bien a près du coup peut être que vous pouvez essayer de retirer vos styles custom pour voir ce que ça donne sans.

totakoko commented 3 months ago

Hm pas sûr car sans style custom on a : image

Et avec : image

C'est principalement pour avoir le header différent + bordure noire au dessous.

garronej commented 3 months ago

@totakoko par contre je vois que vous utilisez MUI mais vous n'avez pas mis en place la configuration d'emotion pour le SSR dans Next.

Vous pouvez suivre le guide ici: https://mui.com/material-ui/integrations/nextjs/#pages-router

Ou encore plus simple vous pouvez appliquer la même configuration que dans le demo setup Next.js Pages Router: https://github.com/garronej/react-dsfr-next-demo (c'est dans pages/_app.tsx et pages/_document.tsx, ici, ici, ici, ici et ici).

Dans le demo setup j'utilise le tooling de tss-react: https://docs.tss-react.dev/ssr/next.js parce que le package @mui/material-nextjs n'existe que depuis recement mais de toute façon ils on repris l'implémentation de TSS donc c'est la même chose.

martinratinaud commented 3 months ago

@garronej Super merci

Pour info, ton 2e ici est plutot ici :-) (il manquait la ligne d'export)

peux tu stp m'expliquer à quoi ça sert cette config ?

Merci d'avance

garronej commented 3 months ago

En fait, avec Next.js, la page envoyée au client a déjà été rendue. Vous n’envoyez pas juste un gros bundle JavaScript, vous envoyez une page déjà construite. Or, quand vous utilisez MUI, la lib utilise Emotion comme moteur de style. Emotion est un moteur de style dynamique qui injecte les styles dans le head du document au moment où le composant est rendu. Il le fait en utilisant l’API DOM. Or, quand vous faites le rendu sur le backend, le DOM n’existe pas. Il faut donc collecter les styles qui sont ajoutés et les mettre manuellement dans le <head /> c'est ce que fait le tooling de tss-react et le module @mui/material-nextjs. Si vous ne le faites pas, vos composants MUI ne sont initialement pas stylés. Vous ne l’avez peut-être pas remarqué, car dès que l’app est hydratée côté client, ces styles sont ajoutés. Concrètement, si vous n’implémentez pas l’injection des styles en SSR, les conséquences sont que les utilisateurs ayant JavaScript désactivé reçoivent une page avec les composants MUI sans style. De plus, vous aurez un genre de scintillement avec la page qui initialement n’est pas complètement stylée pendant quelques frames.

martinratinaud commented 3 months ago

Super explication merci

C'est implémenté

garronej commented 3 months ago

J'ai pas précisé mais vous devez ajouter @emotion/server et tss-react a vos dépendences

garronej commented 3 months ago

Mais honètement c'est un peut relou qu'Emotion n'exporte pas directement son propre utilitaire. J'ai essayer de pousser pour qu'ils le fasse mais ça n'a pas pris. J'ai au moins réussi a convraincre MUI de le faire (enfin j'ai été parmis ceux plutot)