Skip to content

Conversation

garronej
Copy link
Collaborator

@garronej garronej commented Mar 26, 2023

Hi @enguerranws,

Follow up (yet again) of #76.
I merget but I rebased again, I spent another hour or so testing and improving the documentation and I realized that there is still quite a lot of work on this.
Curently there are a lot of way the component can be used that will led to inconsistent state.
There's also some undocumented behaviour.
Plus we haven't writen any tests to make sure users don't get red squigly lines when integrating with form libaries.

The curent implementation (on main) dosen't provide any typesafety but at least it makes it clear to the user that it's just a wrapper arount a default <select />.
With this branch, we give a false sense of security witch is worse.

This PR has potential to improve things but it needs more work. I'd say at least 2hours, maybe more.

Let's schedule another peer programing session.

@garronej
Copy link
Collaborator Author

garronej commented Apr 7, 2023

image

Getting this when using in the regular "controlled no default mode"

@garronej
Copy link
Collaborator Author

garronej commented Apr 7, 2023

No action required, We'll se next peer prog session, I just leave the screenshot here as a reminder.

@enguerranws
Copy link
Collaborator

Oh, I think it's caused by the placeholder, WE should use an empty string as a value if there's a placeholder instead of setting selected prop

@garronej
Copy link
Collaborator Author

garronej commented Apr 8, 2023

@garronej
Copy link
Collaborator Author

garronej commented Apr 8, 2023

The more I use the new Select the more I like it.
We shoudld document the very commun usecase where we are creating a select for for a typed list of entry but the selection can be undefined.

image

The "" as const is what's make it complile

image

@garronej
Copy link
Collaborator Author

garronej commented Apr 8, 2023

When there is an empty string value like
image

We shouldn't see this:

image

It should be "Toutes"

An now I realize that it devinately never makes sence to have a placeholder when the select is controlled and there is a default.

Signed-off-by: Joseph Garrone <joseph.garrone.gj@gmail.com>
@garronej
Copy link
Collaborator Author

I merge as SelectNext since this select is already much better that the current one but we should schedule a session to finish it.

@garronej garronej merged commit e2c4b32 into main Apr 21, 2023
sbourdon13 pushed a commit to sbourdon13/react-dsfr that referenced this pull request Aug 11, 2023
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.

2 participants