-
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] Adds Preview table tab to Data Frames list expanded row #39983
[ML] Adds Preview table tab to Data Frames list expanded row #39983
Conversation
Pinging @elastic/ml-ui |
0c2d424
to
a1a659f
Compare
💔 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.
Looks great! Just added a few questions/suggestions.
...egacy/plugins/ml/public/data_frame/pages/job_management/components/job_list/expanded_row.tsx
Outdated
Show resolved
Hide resolved
|
||
import { DataFramePreviewRequest } from '../../../../common'; | ||
|
||
interface Transform { |
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.
On latest master
you should be able to use DataFrameTransform
from ml/public/data_frame/common/job.ts
.
...egacy/plugins/ml/public/data_frame/pages/job_management/components/job_list/preview_pane.tsx
Outdated
Show resolved
Hide resolved
|
||
async function getPreview() { | ||
try { | ||
const transformData: any = await ml.dataFrame.getDataFrameTransforms(transformId); |
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.
Maybe we can pass on the transform config as a prop and don't need to fetch it again? It's available as .config
on the DataFrameJobListRow
objects.
...egacy/plugins/ml/public/data_frame/pages/job_management/components/job_list/preview_pane.tsx
Outdated
Show resolved
Hide resolved
@peteharverson This is a regression of the removal of automatic date formatting in the wizard done in this PR #39811. Once we do the second part of #39250 we can have human readable dates in the preview table, so this needs to be done in a follow up. |
The headers in the new Preview table (and also in the table inside the wizard) are hyperlinks, which implies sorting functionality, but when I click on them nothing happens. Can sorting be added in? If not, the columns should be set marked as not |
...egacy/plugins/ml/public/data_frame/pages/job_management/components/job_list/preview_pane.tsx
Outdated
Show resolved
Hide resolved
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 🍾
💚 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
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 edits LGTM
…#39983) * create preview tab in DF list expanded row * add route for fetching transform data * sort table columns. fix types * update snapshot * update tab label * add refresh functionality * pass transformConfig as prop instead of fetching
…#40132) * create preview tab in DF list expanded row * add route for fetching transform data * sort table columns. fix types * update snapshot * update tab label * add refresh functionality * pass transformConfig as prop instead of fetching
Summary
Adds
Preview table
tab to Data Frame jobs list expanded row.Note:
Currently, there is a bit a duplication (like the sorting function). The goal is to have the same component used to display the preview table here and in the wizard. For now, I've kept them separate (as refactoring will be required) and copied a bit of what I've needed.
Checklist
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