Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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] Migrate shared Indexing Status component #84571
[Enterprise Search] Migrate shared Indexing Status component #84571
Changes from 9 commits
92db929
d435cf4
6822405
569eb59
7661303
5bf73de
730ac38
e0411e3
e34f038
beb9589
db239d7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question here re: CSS classes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very minor comment, but I don't see the advantage of pulling out
statusPanel
into a separate const since it's not repeated or anything - would suggest just leaving it inlineAlso, do we just show nothing if
percentageComplete
is 100 with no errors? Like, we don't even show a completed bar?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we using these CSS classes? If not, let's lose them for now and re-add them as needed and w/ correct casing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we plan on writing a test for this file?
Also, just curious - not a blocking request for this PR of course (unless people really like the idea), but at some point do we think it would be worth it to rewrite this file as a Kea logic file instead of as a render prop, and have this be an
actions.fetchIndexingStatus
action that updatesvalues.percentageComplete
andvalues.numDocumentsWithErrors
?Doing so has the advantage of not requiring
useRef
forpollingInterval
, for example.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I've been toying with doing that but wondered if I had the time. I think I'll just go ahead and do it. I want to get this merged in so I can test it in Kibana though. Everything but this file has tests and my changes will be a fast-follower to this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sweet, sounds good - thanks Scotty!