-
Notifications
You must be signed in to change notification settings - Fork 8.5k
[ML] DF Analytics creation wizard: show link to results #74025
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] DF Analytics creation wizard: show link to results #74025
Conversation
|
Pinging @elastic/ml-ui (:ml) |
47c6391 to
cf8ed73
Compare
| return ( | ||
| <Fragment> | ||
| <EuiCard | ||
| // @ts-ignore |
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.
nit: would be good to comment why we need this ts-ignore
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.
Moved the inline style to a separate file so this ignore isn't needed anymore. d12ebe4
| const [jobFinished, setJobFinished] = useState<boolean>(false); | ||
| const [currentProgress, setCurrentProgress] = useState< | ||
| | { | ||
| currentPhase: number; |
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.
this should probably have a proper type which can also be used in ProgressStats, where currently any is currently being used.
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.
Updated in d12ebe4
|
|
||
| interface Props { | ||
| currentProgress: any; | ||
| failedJobMessage: any; |
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.
looks like this can be string | undefined
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.
Good spot - updated in d12ebe4
| return ( | ||
| <Fragment> | ||
| <EuiCard | ||
| // @ts-ignore |
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.
why is this ts-ignore needed? a comment here would be helpful
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.
Moved the inline style to a separate file so this ignore isn't needed anymore. d12ebe4
| } | ||
|
|
||
| export function getResultsUrl(jobId: string, analysisType: string) { | ||
| export function getResultsUrl(jobId: string, analysisType: ANALYSIS_CONFIG_TYPE | string) { |
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.
nit, and probably outside the scope of this PR, but it would be good if this only took ANALYSIS_CONFIG_TYPE
it looks like getAnalysisType should return a ANALYSIS_CONFIG_TYPE rather than a string.
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 noticed that, too, but it will involve touching more files so can be done in a separate PR.
|
This has been updated and is ready for a final look when you get a chance. cc @jgowdyelastic, @qn895 |
|
Tested and LGTM 💯 |
💚 Build SucceededBuild metrics@kbn/optimizer bundle module count
page load bundle size
History
To update your PR or re-run it, just comment with: |
jgowdyelastic
left a comment
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
* show view results card once job complete * update types * update types and move css to own file
* master: [ML] DF Analytics creation wizard: show link to results (elastic#74025)
Summary
Related meta issue: #66661
Add view results link next to analytics list link at the end of the creation wizard. Only show the link to the results if the job completed successfully.
When job is still running:
Once job is finished:
Checklist
Delete any items that are not applicable to this PR.