search

codegouvfr / react-dsfr

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

Feature / RadioRichButtons #211

Closed enguerranws closed 8 months ago

enguerranws commented 9 months ago

Todo:

garronej commented 8 months ago

Hey @enguerranws,

Thanks a lot for the PR!

I first I did this review to improve consistency with the rest of the project: https://github.com/codegouvfr/react-dsfr/pull/211/commits/765f23343f8109d18a094819f1dbc67f96252a66

But in the end I did even simpler.

I don't think we need to have a separate RadiRichButton. We can just infer it's rich by the use of illustations.
The drawback is that we can't enforce at the type level that if there is one radio that has an illustration all radio must have one but I think it's really no big deal.

enguerranws commented 8 months ago

I don't think we need to have a separate RadiRichButton. We can just infer it's rich by the use of illustations. The drawback is that we can't enforce at the type level that if there is one radio that has an illustration all radio must have one but I think it's really no big deal.

I thought about that, but I wanted to stick the DSFR's components (where Radio buttons and Radio rich are presented as different components), but that's fine for me!

garronej commented 8 months ago

I thought about that, but I wanted to stick the DSFR's components (where Radio buttons and Radio rich are presented as different components), but that's fine for me!

Oh! Is that so? In that case you probably made the right call.
Anyway, now that it's published do you think I should rollback or keep it like this?

garronej commented 8 months ago

I can rollback the last commit if you think it's best. Idk