-
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] Converts full time range selector control to EUI / React #35074
[ML] Converts full time range selector control to EUI / React #35074
Conversation
Pinging @elastic/ml-ui |
💔 Build Failed |
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 |
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 |
573d2a3
to
1178f2f
Compare
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, just added a question about returning from setFullTimeRange()
.
import { ml } from '../../services/ml_api_service'; | ||
|
||
export function setFullTimeRange(indexPattern: IndexPattern, query: Query) { | ||
return ml |
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 need the return
here?
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.
No the return
isn't needed. I'll remove it.
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.
The return was added in the original version so the promise chain could be used to trigger an update after the time range had been loaded.
I think this is a useful feature and is being currently being used by the new job wizards.
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.
Well spotted @jgowdyelastic . The return
is needed when prepopulating the job wizard settings when cloning a job that was created in the wizards. I have added it back in.
💚 Build Succeeded |
💚 Build Succeeded |
…c#35074) * [ML] Converts full time range selector control to EUI / React * [ML] Remove console log from time range selector service * [ML] Fix linting errors in full time range selector ts files * [ML] Switch to using I18nContext in wrapping directive * [ML] Remove unnecessary return from setFullTimeRange * [ML] Add setFullTimeRange return back in for clone job settings
#35147) * [ML] Converts full time range selector control to EUI / React * [ML] Remove console log from time range selector service * [ML] Fix linting errors in full time range selector ts files * [ML] Switch to using I18nContext in wrapping directive * [ML] Remove unnecessary return from setFullTimeRange * [ML] Add setFullTimeRange return back in for clone job settings
…c#35074) * [ML] Converts full time range selector control to EUI / React * [ML] Remove console log from time range selector service * [ML] Fix linting errors in full time range selector ts files * [ML] Switch to using I18nContext in wrapping directive * [ML] Remove unnecessary return from setFullTimeRange * [ML] Add setFullTimeRange return back in for clone job settings
Summary
Converts the control for automatically setting the time filter to the full time range of the job to EUI / React.
Checklist
For maintainers