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

Add SuggestItem #2061

Closed
andreadelrio opened this issue Jun 19, 2019 · 5 comments
Closed

Add SuggestItem #2061

andreadelrio opened this issue Jun 19, 2019 · 5 comments

Comments

@andreadelrio
Copy link
Contributor

andreadelrio commented Jun 19, 2019

image

image

image

As discussed with Dave, the following would be the approach to replace the interface shown above with Eui components. For reference, that interface can be found at the top of Dashboard and Discover in Kibana.

<div>
 <EuiSuggestInput saveState=”saving”>key : value></EuiSuggestInput>
 <EuiPopover className=”euiSuggest__popover”>
   <EuiSuggestItem />
   <EuiSuggestItem />
   <EuiSuggestItem />
   <EuiSuggestItem />
 </EuiPopover>
</div>

Kibana currently builds this interface using some existing Eui components (namely EuiIcon). Kibana passes the type prop to EuiIcon. It also creates a CSS class {kbnSuggestionItem-- + props.suggestion.type} and calculates color and background-color in SASS for each type (this is for the square which contains the icon on each suggestion). See:
image

In the current implementation each type is linked to a specific color and background-color.

I would like for EuiSuggestItem to offer the user more freedom and allow them pick the color for the icon they're using. For that reason, EuiSuggestItem's type prop would be limited to generating the icon through EuiIcon only. I would add other props to handle color.

Here are the props that I’m considering:

type => to generate the icon using EuiIcon (Kibana currently uses EuiIcon for this and I would like to do the same)
text => primary text / label
description => secondary text (optional)
color => color of the icon. Accepts either our palette colors (primary, secondary ..etc) or a hex value.
bgcolor => background color of the square where the icon is placed. Accepts hex value.

In the case in which the user picks a palette color for color we would auto-generate the background-color for them in SASS. Unless they specify a hex value for bgcolor.

In terms for structure I was thinking of something like this:

<EuiFlexGroup>
 <EuiFlexItem><EuiIcon type={type} /></EuiFlexItem>
 <EuiFlexItem>{text}</EuiFlexItem>
 <EuiFlexItem>{description}</EuiFlexItem>
</EuiFlexGroup>

Please send your comments, suggestions, etc.

@cchaos
Copy link
Contributor

cchaos commented Jun 19, 2019

Great writeup. I think your breakdown looks good, just had a couple suggestions.


I would like for EuiSuggestItem to offer the user more freedom and allow them pick the color for the icon they're using.

This word can be confusing in the way we talk about EUI because "user" to us usually means the end -user of Kibana. However, when what we really mean is someone who is writing code using EUI we tend to refer to as the consumer. It just helps us distinguish between what we might be exposing as a setting to the user versus a prop for the developer.

That said, I agree that allowing the consumer to choose their icon/color combinations is a good idea.

However, instead of splitting it up between type and color props, you could make the type prop an object like:

type: {
  icon: (EuiIcon),
  color: (Selection from a predefined palette, or hex value)
}

Then type.color would be used as the fill color for the icon, and then the component can do the math to tint it for the background. I wouldn't let the consumer supply a background color as we'd be able to ensure proper contrast is achieved.


The prop text is a bit generic, I know that's currently what's being used but maybe there's a better word like label, name or value?


As for using <EuiFlexGroup> and <EuiFlexItem>, I'd be careful forcing yourself into relying on these layout components. Mainly speaking from experience, they are quite finicky and don't really give you the full spectrum of properties you might want to adjust. For example, you can't set the shrink property on it... yet.

I'd suggest just using your own wrapping divs, spans, whatever, and using custom classes and CSS to create your layout. You'll most likely still use the flexbox model, but it gives you the best flexibility.

We try to shy away from custom classes and CSS where possible in consuming apps like Kibana, mainly because it's hard to maintain over there. But here in EUI, it's perfectly good to do so and we even promote it because then you won't have inheritance issues if there's ever a bug in EuiFlexGroup.

@ryankeairns
Copy link
Contributor

ryankeairns commented Jun 19, 2019

Looks great!

+1 regarding not letting the consumer supply a background color; calculating it seems preferable.

There are some structural similarities to other existing components... might be worth poking around in those files: thinking of EuiContextMenuItem , EuiListGroupItem, etc.

I suspect you may end up needing an onClick prop as well, but I'm not certain how the current implementation works. Something to keep an eye out for.

It sounds like you're building this out in pieces and my only other thoughts in looking at this were how to handle things like the number of rows being displayed before scrolling starts (ie. popover height) and if we should let consumers set a width on the text or description. Those feel like parent container concerns, so I only note them here in case the event I have misunderstood the work breakdown.

@andreadelrio
Copy link
Contributor Author

@cchaos thanks for the input, super useful.

However, instead of splitting it up between type and color props, you could make the type prop an object like:

type: {
  icon: (EuiIcon),
  color: (Selection from a predefined palette, or hex value)
}

Then type.color would be used as the fill color for the icon, and then the component can do the math to tint it for the background. I wouldn't let the consumer supply a background color as we'd be able to ensure proper contrast is achieved.

In the case where the consumer passes in a hex value for type.color, how would be go about doing the math for the background color? As far as I've seen, when they pass a hex value we do inline styling (e.g. Avatar, Badge), meaning that hex value never makes it to the scss files where we have the color mixins available (tintOrShade, etc).

@cchaos
Copy link
Contributor

cchaos commented Jun 20, 2019

That's a very good question. I think there are two paths you can follow:

  1. Try to recreate our tint/shade SASS functions in JS. This may/may not be necessary anyway when we implement a CSS in JS solution. Not entire sure.

  2. Use opacity instead of tint/shade functions. You can inherit a background color from the color attribute so you only need to apply the type.color to the wrapping div. So something similar to:

<div class="euiSuggestItem__type" style={{ 'color': type.color }}>
  <div class="euiSuggestItem__typeBackground" />
  <EuiIcon style={{ 'fill': currentColor // This is actually already inherited from .euiIcon }} />
</div>

.euiSuggestItem__type {
  background-color: white // ensures the color's opacity sits on a white background to simulate tint
}

.euiSuggestItem__typeBackground {
  background-color: currentColor;
  opacity: .3; // or whatever value makes sense
}

@thompsongl
Copy link
Contributor

Implemented in #2090

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

No branches or pull requests

4 participants