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

Use EuiBadge for combobox pilling #2

Merged
merged 9 commits into from
Mar 29, 2018

Conversation

snide
Copy link

@snide snide commented Mar 29, 2018

Hey @cjcenizal. Didn't want to slow you down since you're on a tear. I went ahead and made the combo box pills utilize the badge component. That was the original idea when I rebuilt badges, and i just never updated my original prototype.

Should have the following benefits

  • Less maintenance, since badges have since gotten all the abilities of these pills.
  • You can pass a color to them now. Like badges, these can either be a named value or a hex. Primarily I'd like something like this for nathan's tagging system, though im guessing coloring would be helpful in visualization bits as well.
  • Badges should take up less space than the pills, so hopefully there's some more room now.

I'll probably pass something similar to the actual list options as well, so the colors show there in the dropdown, but that part can always come in later. I just figured I'd work through the big piece before we went down a specific road. Did this too.

Nice work with the styling btw. It's hard to get all the pixels to line up with all the wrapping / truncating and what not. Definitely one of the more pixel pushy components we have.

@snide
Copy link
Author

snide commented Mar 29, 2018

Also did some minor cleanup of the grouping to make it pop a bit more.

image

@snide
Copy link
Author

snide commented Mar 29, 2018

Added the colors to the dropdown too.

image

@cjcenizal
Copy link
Owner

Thanks for doing this @snide! Looks great. I'll review the code and merge this in today.

Copy link
Owner

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

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

This is awesome. I think everything looks great, but I think we should either revert the commit that added colors to the options (c35553c), or adjust it so that we have a EuiSwatch component or something similar which can support the different colors you want to show. Then we can merge this and I'll add a renderOption prop to EuiComboBox so that consumers can specify how each option renders. I'll update the example you've added to use this prop with EuiSwatch, to result in the same thing you have here. Wdyt?

@snide
Copy link
Author

snide commented Mar 29, 2018

@cjcenizal I can do that. I won't get to it till the afternoon though if that's ok.

@snide
Copy link
Author

snide commented Mar 29, 2018

Meaning I can make the swatch its own component. Where do you think that should live though? One option I could also do it just make EuiIcon accept hex colors, and then we could use the dot for the color here. Then we don't need an extra component and icons would be a little more flexible.

Lemme know and I'll set it up.

@cjcenizal
Copy link
Owner

Adding this functionality to EuiIcon sounds great, then we can just use EuiHealth in the renderOption definition. I'm not blocked on this so there's no rush. Thanks.

@snide
Copy link
Author

snide commented Mar 29, 2018

OK @cjcenizal, I removed the object_list stuff and made EuiIcon accept hex values. Also added a validator to both badge and icon that checks to make sure the passed color is either one of the palette defined ones or a valid hex value.

Let me know if there's more you need.

@cjcenizal
Copy link
Owner

Looks good! Ready for me to merge this?

@snide
Copy link
Author

snide commented Mar 29, 2018

@cjcenizal yeah, it's all done now.

@cjcenizal cjcenizal merged commit 1dcef1f into cjcenizal:combo-box Mar 29, 2018
cjcenizal pushed a commit that referenced this pull request Apr 28, 2018
…#670)

* use react-virtualized to virtualize combo box options list

* use smaller width and height

* include group label in matching options list

* add better text for example description

* dynamically set width and height

* Massage group title padding. Truncate text instead of wrapping it. Add title attribute to options for usability. (#1)

* remove console.log and fix spelling

* fix problems with settig focus on active option

* more keyboard accessiblity work

* Combo box focus state and text overflow (#2)

* Call setState instead of setting activeOptionIndex directly.

* Clear activeOptionIndex when you click the input.

* Prevent a lot of input from overflowing the container.

* Allow disabled options to be focused but not selected.

* add throttle to incrementActiveOptionIndex to avoid keypresses getting UI out of sync

* rowHeight prop

* remove unneeded const

* fix spacing in example text, fix lodash import

* skip disabled options when using keyboard

* Revert "skip disabled options when using keyboard"

This reverts commit 47fa3ef.
cjcenizal pushed a commit that referenced this pull request Mar 17, 2020
cjcenizal pushed a commit that referenced this pull request Mar 17, 2020
* fixed errors

* updated changelog

* Update CHANGELOG.md

* added preview in docs-section

* Update CHANGELOG.md

* added example file

* Fixing anchor and disabled styles, updating example (#2)

Thanks @cchaos

* added tests

* Update CHANGELOG.md

Co-Authored-By: Caroline Horn <549577+cchaos@users.noreply.github.com>

Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
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