-
Notifications
You must be signed in to change notification settings - Fork 2
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
Feature/select render value #128
base: master
Are you sure you want to change the base?
Conversation
@nicmosc I wouldn't add an extra prop (more things to maintain) and would just allow a <Select options={users.map((u) => ({ value: u.id, label: <Tag>{u.name}</Tag> }))} /> Also goes in line with some of our other APIs like the |
@larsbs i agree that would be easier, but the issue is we use the |
@nicmosc I don't understand, can you link the piece of code that's causing the issue? |
Here is how <select
disabled={disabled}
className={styles.select}
value={value}
onChange={handleOnChange}
{...props}>
{options.map((option) => (
<option key={option.value} value={option.value} disabled={option.disabled}>
{option.label}
</option>
))}
</select> In addition, the html |
@larsbs i completely misread your suggestion, thought we had to pass Either way, the issue with the custom label still applies (the So i'll try to make it work with this approach (checking if the label is string or not) in which case it renders the tag for the value. Still got the issue for the |
@nicmosc let's take a look together later this week, you said you didn't need this in the design system yet right? |
Yeah it's not something that's missing in fact (could have a normal select for now). So we can check that another time. Keeping the PR open though |
As seen on the CRM:
This just a proposition, if not accepted we can always keep the select as it is in the CRM.