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

Feature/select render value #128

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

Conversation

nicmosc
Copy link
Member

@nicmosc nicmosc commented Mar 29, 2020

As seen on the CRM:
image

This just a proposition, if not accepted we can always keep the select as it is in the CRM.

image

@nicmosc nicmosc self-assigned this Mar 29, 2020
@nicmosc nicmosc added the enhancement New feature or request label Mar 29, 2020
@nicmosc nicmosc requested a review from larsbs March 29, 2020 15:00
@larsbs
Copy link
Contributor

larsbs commented Mar 30, 2020

@nicmosc I wouldn't add an extra prop (more things to maintain) and would just allow a ReactNode to go in the label of the passed options. Since you probably have to map already (as we removed valueKey and labelKey), you might as well return a component instead of a string.

<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 Table component.

@nicmosc
Copy link
Member Author

nicmosc commented Mar 30, 2020

@larsbs i agree that would be easier, but the issue is we use the label for the select option as well, which won't be rendered if we use a React.Node. My other idea was to have an additional value field instead of the renderLabel, where you can there pass the custom component (though that means you have to extract the label yourself, rather than having it come from a function.
If none of these solutions are ideal we can always scrap this and stick to "standard" select

@larsbs
Copy link
Contributor

larsbs commented Mar 30, 2020

@nicmosc I don't understand, can you link the piece of code that's causing the issue?

@nicmosc
Copy link
Member Author

nicmosc commented Mar 30, 2020

Here is how Select works, you pass value and options where each option object has a value property which matches the currently given value (standard select implementation). If the value has type React.Node it won't be possible to accurately match the option value to the current value given to the Select.

<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 select value doesn't accept anything other than string really: if you try to pass a React.Node to the select nothing will render. We could go the full custom route, but then it gets messy to keep supporting mobile which relies on the native behaviour

@nicmosc
Copy link
Member Author

nicmosc commented Mar 30, 2020

@larsbs i completely misread your suggestion, thought we had to pass value lol

Either way, the issue with the custom label still applies (the option element can't render anything other than text
image
image

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 label displayed within the options though. Do we somehow "extract" the text value of the label?

@nicmosc
Copy link
Member Author

nicmosc commented Mar 30, 2020

@larsbs tried a different way here #130
But for some reason the innerHTML is not being rendered in the option, though if you log it you see all the values have the correct dom elements assigned:
image

@larsbs
Copy link
Contributor

larsbs commented Mar 30, 2020

@nicmosc let's take a look together later this week, you said you didn't need this in the design system yet right?

@nicmosc
Copy link
Member Author

nicmosc commented Mar 30, 2020

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

@nicmosc nicmosc added the on hold If the feature is temporarily blocked from merging label Apr 6, 2020
@nicmosc nicmosc changed the title render custom label on top of select Feature/Select render value Apr 10, 2020
@nicmosc nicmosc changed the title Feature/Select render value Feature/select render value Apr 10, 2020
@nicmosc nicmosc changed the base branch from beta to master April 23, 2020 09:27
@nicmosc nicmosc added the minor Increment the minor version when merged label May 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request minor Increment the minor version when merged on hold If the feature is temporarily blocked from merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants