codegouvfr / react-dsfr

🇫🇷 The Frech Government Design system React toolkit
https://react-dsfr.codegouv.studio
MIT License
403 stars 49 forks source link

SelectNext n'accepte pas d'option avec une value vide #280

Closed martinratinaud closed 1 month ago

martinratinaud commented 1 month ago

Pour France Chaleur Urbaine, nous avons besoin de définir un SelectNext qui a une option avec une valeur vide

Ceci n'est pas possible car le composant actuel défini une option disabled par défaut avec une valeur vide aussi

Je propose 3 options:

Merci pour vos retours

totakoko commented 1 month ago

+1

Je vote pour la 3e solutions histoire d'avoir le moins d'impact sur les projets qui l'utilisent.

ddecrulle commented 1 month ago

Est-ce possible de détailler le besoin ?

garronej commented 1 month ago

Salut @martinratinaud,

Comme @ddecrulle, je serais intéressé par avoir le cas d'usage. Pour la proposition 1 : Pour un composant plus headless, il y a déjà le composant Select qui délègue plus de responsabilités au développeur. Pour la proposition 2, je ne comprends pas très bien, il y a déjà une option placeholder. Ok pour la proposition 3, elle n'a pas d'impact sur l'API, je vais publier une candidate.

Le composant select a l'air simple car tous les cas pris individuellement sont simples, mais parvenir à avoir un composant qui couvre tous les cas d'usage et en plus produise une bonne inférence de types est très complexe. Comme vous pouvez le voir ici sur les compile time tests : https://github.com/codegouvfr/react-dsfr/blob/c60ad4ab9ed43309218b333230971234dd08a603/test/types/Select.tsx

D'ailleurs, MUI a renoncé à fournir de l'inférence de type sur leur composant de formulaire...

garronej commented 1 month ago

Vous pouvez essayer @codegouvfr/react-dsfr@1.9.30-rc.1 je vous laisse me dire si ça marche pour vous.

garronej commented 1 month ago

Oui je comprend le usecase maintenant j'ai retravailler SelectNext et mis a jour la documentation storybook du component.

totakoko commented 1 month ago

@garronej J'ai pu tester ta RC. Ça fonctionne bien car on peut maintenant définir nous-mêmes la propriété avec valeur vide. Cad mettre une option avec label "Tous" et value "" par exemple.

Après, pour notre migration, j'ai directement utilisé le SelectNext plutôt que le Select classique comme indiqué dans la doc. Est-ce qu'il faudrait qu'on repasse sur le Select classique ?

garronej commented 1 month ago

Non non c'est bien d'avoir utiliser SelectNext.
J'ai clarifier dans le Storybook:

image

J'ai aussi rajouter une note pour la valeur vide.

image
garronej commented 1 month ago

Bon aller je release alors :)

totakoko commented 1 month ago

C'est bon pour nous. Merci @garronej !

garronej commented 1 month ago

Merci d'avoir fait remonter. Dsl d'avoir mis du temps a comprendre la pertinence

totakoko commented 1 month ago

Y'a pas de souci. C'est normal de questionner et bien comprendre le besoin ;)

martinratinaud commented 1 month ago

Merci beaucoup pour la rapidité dans le fix, c'est super appréciable et ça donne envie de vous remonter plein d'autres améliorations ;-)