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 Suggest component #2270

Merged
merged 73 commits into from
Sep 13, 2019
Merged

Add Suggest component #2270

merged 73 commits into from
Sep 13, 2019

Conversation

andreadelrio
Copy link
Contributor

@andreadelrio andreadelrio commented Aug 28, 2019

Fixes #1938
Fixes #2270

Summary

This PR adds the EuiSuggest component and the corresponding docs.

This component provides a way to display suggestions using a text field. Behind the scenes it uses EuiSuggestItem to display each suggestion following a very similar format to the one currently present in Kibana.

The consumer is to handle what the current status of EuiSuggest is. The supported status are notYetSaved, saved, noNewChanges and isLoading. Custom tooltip content is supported for these.

EuiSuggest can also have elements prefixed and/or appended to it. For this, there's the prefix and append props which are both PropTypes.node.

image
image

Checklist

  • Checked in dark mode
  • Checked in mobile
  • Checked in IE11 and Firefox
  • Props have proper autodocs
  • Added documentation examples
  • Added or updated jest tests
  • Checked for breaking changes and labeled appropriately
    - [ ] Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@andreadelrio
Copy link
Contributor Author

@cchaos I think I've addressed most of your comments. Still not 100% happy with the behavior of the input on the Saved Queries example (it's a bit jumpy). Additionally, I added a handler for a click on a suggestion.

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Still not 100% happy with the behavior of the input on the Saved Queries example (it's a bit jumpy).

I think we can continue to tackle that in a separate PR. For now, I'm concentrating on the actual EuiSuggest and it's components.


Sorry still finding a few things since this is such a large PR.

When the status is unchanged it feels odd that the focus state doesn't extend the full length of the input .

src-docs/src/views/suggest/saved_queries.js Outdated Show resolved Hide resolved
src/components/suggest/suggest_input.js Outdated Show resolved Hide resolved
src/components/suggest/suggest.js Outdated Show resolved Hide resolved
src-docs/src/views/suggest/saved_queries.js Outdated Show resolved Hide resolved
src-docs/src/views/suggest/suggest_example.js Outdated Show resolved Hide resolved
src-docs/src/views/suggest/suggest_example.js Outdated Show resolved Hide resolved
src/components/suggest/_suggest_input.scss Outdated Show resolved Hide resolved
src-docs/src/views/suggest/suggest.js Show resolved Hide resolved
src/components/suggest/suggest_item.js Outdated Show resolved Hide resolved
const statusElement = status !== 'isLoading' && (
<EuiToolTip
position="left"
content={tooltipContent}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can still supply some good defaults in the component itself for the tooltip instead of forcing that on the consumer. Probably just add a few to the statusMap but allow tooltipContent to override.

Suggested change
content={tooltipContent}
content={tooltipContent || statusMap[status].tooltip}

The || will evaluate the first option first and if it is false, move to the next option.

@andreadelrio andreadelrio requested a review from cchaos September 5, 2019 23:32
@andreadelrio
Copy link
Contributor Author

@cchaos managed to fix the styling when using button and made the other changes too. Component feels a bit cleaner now.

Copy link
Contributor

@cchaos cchaos 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 really starting to come together nicely! My comments now are super nit picky.

src-docs/src/views/suggest/saved_queries.js Outdated Show resolved Hide resolved
src/components/suggest/suggest.js Outdated Show resolved Hide resolved
src/components/suggest/suggest.js Outdated Show resolved Hide resolved
src/components/suggest/suggest.js Outdated Show resolved Hide resolved
src/components/suggest/_suggest_item.scss Outdated Show resolved Hide resolved
src/components/suggest/suggest_input.js Outdated Show resolved Hide resolved
src/components/suggest/suggest_input.js Outdated Show resolved Hide resolved
@andreadelrio andreadelrio requested a review from cchaos September 9, 2019 16:04
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Great! LGTM, but we should get an engineer to do a pass as well. @chandlerprall can you review this one?

src-docs/src/views/suggest/suggest.js Outdated Show resolved Hide resolved
src-docs/src/views/suggest/suggest.js Outdated Show resolved Hide resolved
src/components/suggest/suggest.js Outdated Show resolved Hide resolved
src/components/suggest/suggest.js Outdated Show resolved Hide resolved
src/components/suggest/suggest.js Outdated Show resolved Hide resolved
src/components/suggest/suggest.js Outdated Show resolved Hide resolved
constructor(props) {
super(props);

this.state = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly this state initialization can move out of the constructor.

src/components/suggest/suggest_input.js Outdated Show resolved Hide resolved
src/components/suggest/suggest_input.js Outdated Show resolved Hide resolved
src/components/suggest/suggest_item.js Outdated Show resolved Hide resolved
@andreadelrio
Copy link
Contributor Author

@chandlerprall thanks for your review. I've made changes following your suggestions.

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

LGTM! Pulled locally and tested in docs

customLabel: '',
};
}
state = {
Copy link
Contributor

Choose a reason for hiding this comment

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

@chandlerprall Should all the states have been pulled out of the constructor?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep! This change is great to see

Copy link
Contributor

Choose a reason for hiding this comment

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

TIL, thx

@snide
Copy link
Contributor

snide commented Sep 11, 2019

This will need a changelog update, but is now safe to merge. Congrats 🎉

@snide
Copy link
Contributor

snide commented Sep 13, 2019

@andreadelrio let's get this in 🚀

@andreadelrio andreadelrio merged commit f5d0aa5 into elastic:master Sep 13, 2019

const suggestionList = suggestions.map((item, index) => (
<EuiSuggestItem
type={item.type}

Choose a reason for hiding this comment

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

className is not mapped here. Shall I submit a PR? @andreadelrio

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.

New patterns for KQL bar
6 participants