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

[Enterprise Search] Convert IndexingStatus to use logic for fetching #84710

Conversation

scottybollinger
Copy link
Contributor

@scottybollinger scottybollinger commented Dec 1, 2020

Summary

This PR converts the IndexingStatus component to use a newly added logic file instead of the legacy IndexingStatusFetcher component. Because this is the egg and we have no chicken yet, I have pushed a POC of this to ent-search in case QA is desired before merging.

image

Checklist

@scottybollinger scottybollinger added Feature:Plugins release_note:skip Skip the PR/issue when compiling release notes v7.11.0 labels Dec 1, 2020
@scottybollinger scottybollinger requested a review from a team December 1, 2020 21:34
onComplete is not optional so these if blocks can be consolidated
Copy link
Contributor

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

Mostly super minor comments, feel free to push back on anything that's too annoying or might take too long to do

Also great catch on the recent conditional refactor! ✨

Comment on lines +51 to +53
expect(wrapper.find(EuiPanel)).toHaveLength(1);
expect(wrapper.find(IndexingStatusContent)).toHaveLength(1);
expect(fetchIndexingStatus).toHaveBeenCalled();
Copy link
Contributor

Choose a reason for hiding this comment

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

Love how much easier this is to test w/o the render prop!! 🎉

Copy link
Contributor

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

Refactor looks great!! Thanks so much for always improving our code Scotty! ✨

In ent-search, we use Rails helpers to generate paths. These were in the form of routes.whateverPath(). We passed these method to the IndexingStatus component to generate the app-specific rotues in the shared component.

In Kibana, we will not have these generators and should instead pass the path strings directly
Copy link
Contributor

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

New commit LGTM 👍

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@scottybollinger scottybollinger merged commit b6913a3 into elastic:master Dec 2, 2020
@scottybollinger scottybollinger deleted the scottybollinger/indexing-status-logic branch December 2, 2020 19:34
scottybollinger added a commit to scottybollinger/kibana that referenced this pull request Dec 2, 2020
…lastic#84710)

* Add IndexingStatusLogic

* Replace IndexingStatusFetcher with logic

* Refactor out unnecessary conditional

onComplete is not optional so these if blocks can be consolidated

* Misc styling - destructuring and typing

Co-authored-by: Constance <[email protected]>

* Misc styling - imports

Co-authored-by: Constance <[email protected]>

* Remove div

* Refactor test

* Replace method with string for statusPath

In ent-search, we use Rails helpers to generate paths. These were in the form of routes.whateverPath(). We passed these method to the IndexingStatus component to generate the app-specific rotues in the shared component.

In Kibana, we will not have these generators and should instead pass the path strings directly

Co-authored-by: Constance <[email protected]>
scottybollinger added a commit that referenced this pull request Dec 2, 2020
…84710) (#84825)

* Add IndexingStatusLogic

* Replace IndexingStatusFetcher with logic

* Refactor out unnecessary conditional

onComplete is not optional so these if blocks can be consolidated

* Misc styling - destructuring and typing

Co-authored-by: Constance <[email protected]>

* Misc styling - imports

Co-authored-by: Constance <[email protected]>

* Remove div

* Refactor test

* Replace method with string for statusPath

In ent-search, we use Rails helpers to generate paths. These were in the form of routes.whateverPath(). We passed these method to the IndexingStatus component to generate the app-specific rotues in the shared component.

In Kibana, we will not have these generators and should instead pass the path strings directly

Co-authored-by: Constance <[email protected]>

Co-authored-by: Constance <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Plugins release_note:skip Skip the PR/issue when compiling release notes v7.11.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants