Skip to content
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

refactor(look&feel): select native #888

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

fffan64
Copy link
Contributor

@fffan64 fffan64 commented Feb 17, 2025

removed react-select dep to implement native select instead

as you know, options are not (yet!) stylable as they are managed by os

image

image

breaking from current implementation (value returned is now normal value of options instead of previous {label; value} of react-select)

@fffan64 fffan64 force-pushed the refactor/select-native branch 2 times, most recently from 86eb66f to f9276ed Compare February 17, 2025 15:33
@fffan64 fffan64 force-pushed the refactor/select-native branch from f9276ed to fcdb314 Compare February 17, 2025 15:38
@fffan64 fffan64 force-pushed the refactor/select-native branch from fcdb314 to a40df1b Compare February 18, 2025 08:12
jmevel
jmevel previously approved these changes Feb 19, 2025
</option>
)}
{options.map((o) => (
<option key={o.label} value={o.value}>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On ne devrait pas mettre la clé sur la value plutot ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

je c pas, l'un ou l'autre rien ne garanti que c unique de toute façon

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@buddyvegas m'a conseille de faire combo label - value.

@fffan64 fffan64 force-pushed the refactor/select-native branch 2 times, most recently from 3423f63 to c830a68 Compare February 20, 2025 14:50
a-pourrier
a-pourrier previously approved these changes Feb 20, 2025
</option>
)}
{options.map((o) => (
<option key={`${o.label}-${o.value}`} value={o.value}>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

au risque de changer l'implem mias pourquoi ne pas ce rattrocher de la syntaxe html natif a savoir de d'avoir un composant Options

<Select>
  <Option>option 1 </Option>
  <Option>option 2 </Option>
  <Option>option 3 </Option>
  <Option>option 3 </Option>
</Select>
```

ce qui permet à nous de plus gérer la key et serait au consommateur de gérer

qu'en penses tu ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

en faisant comme ca tu peux pas avoir de placeholder

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ca serait au consommateur de le gérer parce que le placeholder sur un sélection c'est une option avec une value vide

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, j'ai change pour aller dans ce sens

@fffan64 fffan64 dismissed stale reviews from a-pourrier and GuillaumeKESTEMAN via df4e23e February 20, 2025 15:31
@fffan64 fffan64 force-pushed the refactor/select-native branch from c830a68 to df4e23e Compare February 20, 2025 15:31
@fffan64 fffan64 force-pushed the refactor/select-native branch from df4e23e to ff383c9 Compare February 20, 2025 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants