-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[ML] Kibana Management Jobs list: Ensure proper title, tagline, and link to documentation #43418
[ML] Kibana Management Jobs list: Ensure proper title, tagline, and link to documentation #43418
Conversation
Any feedback on the documentation links or particular language used is always welcome 😄 |
💚 Build Succeeded |
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.
LGTM - but limited review of screenshots only.
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.
We need to keep the useRefreshInterval()
at the top level, happy to discuss alternatives.
// Call useRefreshInterval() after the subscription above is set up. | ||
useRefreshInterval(setBlockRefresh); | ||
|
||
if (isManagementTable === false) { |
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.
Hooks need to be at the top level, we cannot put them inside conditions unfortunately https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level.
Maybe an alternative is to pass isManagementTable
on to the hook and act on it there?
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.
That makes sense normally, but in this case isManagementTable
value is never changing so it wouldn't change the order of the hooks being called. 🤔
I'm happy to change it though since maybe it does set an example for a pattern we wouldn't want to follow most of the time.
💚 Build Succeeded |
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.
LGTM
💔 Build Failed |
💔 Build Failed |
retest |
💚 Build Succeeded |
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.
Latest changes LGTM
@cchaos - Thank you for taking a look! 😄 The We will be updating this view in subsequent iterations - it might be more appropriate then, depending on what other action buttons we add, to update the style for the |
…ink to documentation (#43418) (#43585) * Update KM jobs list title and add tag line and link to docs * add spaces column to analytics table for use in Kibana management * add refresh button to kibana management analytics jobs list * ensure refresh button does not cause bounce when loading * move refreshInterval hook to page component * add default value for analyticsTable prop
Pinging @elastic/ml-ui |
Summary
Follow up to #43151
Machine Learning Jobs
View machine learning analytics and anomaly detection jobs.
Spaces
column to analytics jobs tableChecklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.- [ ] Documentation was added for features that require explanation or tutorialsFor maintainers
- [ ] This was checked for breaking API changes and was labeled appropriately- [ ] This includes a feature addition or change that requires a release note and was labeled appropriately