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

fix(dropdown): use itemToElement for selected label if not null #4977

Merged
merged 6 commits into from
Jan 9, 2020
Merged

fix(dropdown): use itemToElement for selected label if not null #4977

merged 6 commits into from
Jan 9, 2020

Conversation

jendowns
Copy link
Contributor

@jendowns jendowns commented Jan 8, 2020

Closes #4913

As described in #4913, if a someone utilizes itemToElement to render a custom component inside dropdown items, that custom component isn't used when you select a dropdown item. Instead, a selected dropdown item is made into a string regardless of implementation.

You can demonstrate this here: http://react.carbondesignsystem.com/?path=/story/dropdown--items-as-components
(Select one of the custom options with blue text, and you will see that the blue option number isn't shown as "selected" -- the option text is all black, meaning the custom component for the option was dropped)

So this PR changes the "selected item" label rendering logic to include itemToElement if it is used.

Changelog

Changed

  • use itemToElement for "selected" dropdown item label, if itemToElement is not null... otherwise, do what was done before with rendering as a string.

@netlify
Copy link

netlify bot commented Jan 8, 2020

Deploy preview for the-carbon-components ready!

Built with commit be5f3dd

https://deploy-preview-4977--the-carbon-components.netlify.com

@netlify
Copy link

netlify bot commented Jan 8, 2020

Deploy preview for carbon-components-react failed.

Built with commit be5f3dd

https://app.netlify.com/sites/carbon-components-react/deploys/5e17b41d8a37740008a3af0e

@netlify
Copy link

netlify bot commented Jan 8, 2020

Deploy preview for carbon-elements failed.

Built with commit be5f3dd

https://app.netlify.com/sites/carbon-elements/deploys/5e17b41dcf4b450009e9b64b

@ghost ghost requested review from aledavila and joshblack January 8, 2020 22:08
Copy link
Contributor

@asudoh asudoh left a comment

Choose a reason for hiding this comment

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

LGTM 👍 - Thanks @jendowns!

Copy link
Contributor

@abbeyhrt abbeyhrt left a comment

Choose a reason for hiding this comment

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

🎉 Thanks @jendowns!

@asudoh asudoh merged commit 057e35c into carbon-design-system:master Jan 9, 2020
@joshblack
Copy link
Contributor

joshblack commented Jan 10, 2020

I'm a little confused, I had interpreted that itemToElement was meant for rendering an option in the list of options, is that not the case?

@jendowns
Copy link
Contributor Author

jendowns commented Jan 10, 2020

@joshblack -- when you say "rendering an option" do you mean using a custom component as an option, rather than just text? That's what my interpretation of it is. So if you put blue text or something in the list of options and then select that option, now that "selected" option will be blue like it was in the list. (Previously the selected option was just rendered as a string regardless of if you did something custom)

@joshblack
Copy link
Contributor

@jendowns yeah, I think it should be meant only for rendering an option in a list. If we support custom rendering for the value on top of the field, we won't be able to control things like contrast or could end up with violations if people decide to put things other than text in that location. Could easily be misreading though, let me know.

@jendowns
Copy link
Contributor Author

Oh yeah that's a good point @joshblack -- do you think that same concern should apply to the list of options as well, which could for example have a custom option that doesn't meet contrast requirements, etc.?

Also would you prefer I revert / undo this?

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.

Dropdown component doesn't support component label.
4 participants