Add Suggest Item

[andreadelrio]

Summary

Add SuggestItem component. Covers #2061

Checklist

• This was checked in mobile
• This was checked in IE11
• This was checked in dark mode
• Any props added have proper autodocs
• Documentation examples were added
• A changelog entry exists and is marked appropriately
  - [ ] This was checked for breaking changes and labeled appropriately
• Jest tests were updated or added to match the most common scenarios
  - [ ] This was checked against keyboard-only and screenreader scenarios

[chandlerprall]

The layout prop documentation is a bit confusing for me -

Use layout to change the distribution of the EuiSuggestItem elements.

Suggests to me that the order of the content will change, or its organization will be different, as opposed to simply expanding to fit content. This should probably call out or demonstrate that inline doesn't just allow longer labels but will shorten to fit smaller content as well.

Also, I don't find the inline and setColumns values meaningful. Is this functionality expected to be expanded in the future?

[andreadelrio]

@chandlerprall Good point about the layout prop. So that prop was initially called expandLongLabel. However, while discussing with Dave he suggested to make it more flexible and call it layout instead. My understanding is that we could eventually support more layouts. For example something like this:

@snide is this inline with what you were proposing?

[snide]

Yep. In general whenever we're making alternate displays I think it's smart to tie them to a more flexible enum rather than a Boolean. We tend to add more over time. Whether layout is the correct propname is a diff issue.

…

On Fri, Jul 5, 2019, 11:12 AM Andrea Del Rio ***@***.***> wrote: @chandlerprall <https://github.com/chandlerprall> Good point about the layout prop. So that prop was initially called expandLongLabel. However, while discussing with Dave he suggested to make it more flexible and call it layout instead. My understanding is that we could eventually support more layouts. For example something like this: [image: image] <https://user-images.githubusercontent.com/4016496/60738541-8a992c80-9f13-11e9-82f3-0388b427f132.png> @snide <https://github.com/snide> is this inline with what you were proposing? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2090?email_source=notifications&email_token=AACPHJ42OGNJSQS6GPOCGQDP56FKFA5CNFSM4H5MKMWKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZKCMOI#issuecomment-508831289>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACPHJ47L2RGL3NJMT24IPTP56FKFANCNFSM4H5MKMWA> .

[thompsongl]

Just a note: I've committed to helping @andreadelrio convert this to TypeScript after we're set with the functionality and design of the component. So this is purposefully TS-less, and there will (likely) be a follow-up PR.

[chandlerprall]

Dave he suggested to make it more flexible and call it layout instead. My understanding is that we could eventually support more layouts. For example something like this:

I agree with allowing future options with an enum of values instead of a boolean. However, I don't find the current choices of inline and setColumns to be meaningful to an engineer using this component - I think the names are too attached to the actual CSS/DOM implementation instead of the presentation intent.

[cchaos]

The main things I notice is that we should try to avoid the hard-coded width of the label and figure out a more flexible way to handle the sizing.

Also, the contrast in dark mode is not great.

We'll have to figure out a better way to handle the "faded" version of the color.

[cchaos]

One other visual thing: Can you find a way to get the baselines of the two columns to line up?

[cchaos]

Looks like there's some instances that also need a key prop as error in console shows:

[andreadelrio]

Thanks everyone for your feedback so far. Here are some things I'm considering:

1. To ensure good color contrast: only allow the consumer to choose from a limited set of colors (mix of our core palette plus some viz colors). This is following what we currently do for EuiToken. That way we'd have something like:

2. For the layout prop to have two values fixedWidthLabel and expandLabel. Alternative name for layout could be labelDisplay. By default, when SuggestItem doesn't have a description the layout goes to expandLabel. See:

3. Where to place SuggestItem in the docs? SuggestItem will be part of SuggestInput so, as suggested, we could place it under Forms > Suggest Input > Suggest Item. Since SuggestInput will be a later and separate PR we could just hide SuggestItem's docs for now.

Please send your comments!

[cchaos]

1. only allow the consumer to choose from a limited set of colors

Yep, I think that's a good approach. We could still allow someone to pass a particular color, but that color would then be the full background color and we'd just calculate if the text should be white or black. Similar to how we handle in EuiAvatar.

2. Alternative name for layout could be labelDisplay

I like that name better. You can even simplify the options to be:

labelDisplay: 'fixed' | 'expand'

3. Where to place SuggestItem in the docs?

I'd assume, based on our previous patterns, that you'd have one large encompassing EuiSuggest (folder name: suggest) where the EuiSuggestInput is the actual component housing the search bar and then EuiSuggest would be combining EuiSuggestInput with a EuiPopover housing the EuiSuggestItems.

[chandlerprall]

Fully agree with Caroline on 2!

[cchaos]

This is already getting much simpler. I think you're in the home stretch. The first big PR always takes a while.

[andreadelrio]

I went ahead and replaced the color options to use the vis palette only. I think it looks better! Thanks for that suggestion.

Regarding the expand label situation. I decided to add a min-width:50% to the label that way if the "expanded" label would be less that 50% it would still take up 50%. See results below. First item has labelDisplay='fixed' and second and third items have labelDisplay='expand'

[cchaos]

Final comments, I swear. Mostly around docs and prop descriptions/requirements. I labelled a few optional. But functionally, stylistically, SASS-wise it all looks great!!!

[myasonik]

I haven't had a chance to thoroughly review this yet but I wanted to check if keyboard and screen reader testing was on your radar? I noticed that it's crossed out but, especially as a reusable pattern, I think we have to make sure we're raising the bar on one-off implementations.

[cchaos]

@myasonik Yes, it typically is an item we make sure to check off vs cross-off. But this is a smaller piece of a large component that we're piece-mealing in. When the full component is in review we'll be sure to do a full keyboard nav test as well.

[cchaos]

Looks great! Just saw one duplicated class name and you'll need a changelog entry.

[cchaos]

You no longer need euiSuggestItem__label here because you've added it to labelDisplayClass at line 41.

Suggested change

	<span className={`euiSuggestItem__label ${labelDisplayClass}`}>
	<span className={labelDisplayClass}>

[chandlerprall]

One small ask, but otherwise LGTM

[chandlerprall]

As this is merging into master, can we comment out this first section or update it with a coming soon message?

[cchaos]

I think the idea is just to comment it out from the routes file for now instead.