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

[Graph] EUI-ification of search bar #45351

Merged
merged 21 commits into from
Sep 19, 2019

Conversation

flash1293
Copy link
Contributor

@flash1293 flash1293 commented Sep 11, 2019

Summary

Related #44103

This PR moves the search bar and the index pattern picker to React and Typescript. Additionally a modal to pick the initial index pattern is shown when a new workspace is opened. Both modal and popover use the shared SavedObjectFinder component. This will get more helpful in the future when saved searches can also be picked as data source.

Screenshot 2019-09-11 at 12 13 23

The vertex manager is not changed in this PR, this will be done separately.

App arch reviewer

I slightly extended the API of SavedObjectFinder to also return the actual saved object instance instead of just the id. This avoids an additional lookup of the saved object and will make it possible to not fetch the list of all index patterns in Graph in a further refactoring PR.

It's a bit ugly that the onChoose callback now provides four parameters - if you feel strongly about this I can also clean up the existing usages with a nicer API.

@flash1293 flash1293 marked this pull request as ready for review September 12, 2019 09:45
@flash1293 flash1293 requested review from a team as code owners September 12, 2019 09:45
@flash1293 flash1293 requested a review from kertal September 12, 2019 09:45
@elastic elastic deleted a comment from elasticmachine Sep 13, 2019
@elastic elastic deleted a comment from elasticmachine Sep 13, 2019
@elastic elastic deleted a comment from elasticmachine Sep 13, 2019
@elastic elastic deleted a comment from elasticmachine Sep 13, 2019
@elastic elastic deleted a comment from elasticmachine Sep 13, 2019
@elastic elastic deleted a comment from elasticmachine Sep 13, 2019
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

app arch changes LGTM

@kertal
Copy link
Member

kertal commented Sep 17, 2019

There's one thing to investigate: when selecting an index pattern there seems to be a sorting issue, in my case I've got a kibana_sample_data_ecommerce and timestamp-* changing position for a brief moment.

@kertal I can't reproduce this, for me it's just showing the list directly. If you can reproduce it, could you open a separate issue for that? The saved object finder is a component also used in other apps, so I'd like to make this fix separately.

I will!

@kertal kertal self-requested a review September 17, 2019 13:32
Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

Code LGTM!

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@cchaos
Copy link
Contributor

cchaos commented Sep 18, 2019

Not sure what you mean - it's the same component, just rendered in the popover. It can grow to make room for all of the controls and also be configured to display other data sources

It still feels like such an uneccesary tight spot. Since it's the exact same UI, I would just continue to use the modal instead.

@flash1293
Copy link
Contributor Author

@cchaos as discussed offline, I changed it to always use the modal.

@monfera
Copy link
Contributor

monfera commented Sep 18, 2019

@cchaos it's more of a comment about the visuals— the underscore at the bottom of the search field feels a bit like a progress bar during the animation, while the real progress bar upon searching is on the very top of the screen (I like that the progress bar is subtle; resembles the image loading bars in Front row to Fashion Week though could maybe be slightly more prominent). Maybe the field selection signal outcompetes the real progress bar for the user's grasp of what's a progress bar (IOW the field selection underline stays solid upon search and the user may think it was instantaneous). Feels like this specific type of field selection animation was designed for shorter fields, and/or ones that don't trigger a wait time.

underline

Copy link
Contributor

@monfera monfera left a comment

Choose a reason for hiding this comment

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

Looks beautiful and solid, great work Joe and Caroline!

Minor: I get rows of this upon using the search field, probably you know if it's something new or unrelated to the change:
image

@cchaos
Copy link
Contributor

cchaos commented Sep 18, 2019

In reference to the search input, that's how all our inputs behave for the focus state it's just way more pronounced on such a wide input because the animation has much further to go.

The progress indicator at the top of the page should actually be replaced with the search input's own isLoading state:

Screen Shot 2019-09-18 at 11 32 41 AM

@elasticmachine
Copy link
Contributor

💔 Build Failed

@flash1293
Copy link
Contributor Author

@cchaos

The progress indicator at the top of the page should actually be replaced with the search input's own isLoading state:

It's showing both currently. I think the progress indicator at the top of the page is a low level thing that will be removed once I move the data fetching over in a separate PR.

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.

Sass is good

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@flash1293 flash1293 merged commit c739cb6 into elastic:master Sep 19, 2019
@flash1293 flash1293 deleted the graph/indexpattern_modal branch September 19, 2019 09:20
flash1293 added a commit to flash1293/kibana that referenced this pull request Sep 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Graph Graph application feature release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.5.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants