-
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] DF Analytics / Transforms: Fix job row actions menu invalid DOM nesting warning #74499
[ML] DF Analytics / Transforms: Fix job row actions menu invalid DOM nesting warning #74499
Conversation
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.
Tested and confirmed the DOM errors are no longer seen. Just left a couple of minor comments.
expect(typeof actions[2].render).toBe('function'); | ||
expect(typeof actions[3].render).toBe('function'); | ||
// Using `any` for the callback. Somehow the EUI types don't pass | ||
// on the `data-test-subj` attribute correctly. We're interesting |
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.
Typo! Should be interested
.
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.
Fixed in 74071d9.
color="text" | ||
size="xs" | ||
onClick={clickHandler} | ||
iconType="tableOfContents" |
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.
I think we should switch this icon to be consistent with the one in the main DFA jobs list - tableDensityNormal
.
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.
Consolidated button icons in c9fc1b2.
canStartStopDataFrameAnalytics={canStartStopDataFrameAnalytics} | ||
/> | ||
), | ||
available: (i: DataFrameAnalyticsListRow) => !isDataFrameAnalyticsRunning(i.stats.state), |
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.
I think there needs to be a check here for the job being in a failed state to ensure that we don't lose the fix from (https://github.com/elastic/kibana/pull/74710/files)
E.g. !isDataFrameAnalyticsFailed(i.stats.state)
.
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 catch! I carried over the changes related to the stop action rendering but missed this one. Fixed in c9fc1b2.
const action: DataFrameAnalyticsListAction = useMemo( | ||
() => ({ | ||
name: () => <StopActionName isDisabled={!canStartStopDataFrameAnalytics} />, | ||
available: (i: DataFrameAnalyticsListRow) => isDataFrameAnalyticsRunning(i.stats.state), |
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.
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 catch! I carried over the changes related to the stop action rendering but missed this one. Fixed in c9fc1b2.
Looks good overall. Gave it a test and looks good aside from the comments I left. |
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 ⚡
x-pack/plugins/ml/public/application/explorer/explorer_charts/explorer_charts_container.js
Show resolved
Hide resolved
x-pack/plugins/ml/public/application/jobs/jobs_list/components/job_actions/results.js
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 edits LGTM
/** | ||
* Temp component to have Clone job button with the same look as the other actions. | ||
* Replace with {@link getCloneAction} as soon as all the actions are refactored | ||
* to support EuiContext with a valid DOM structure without nested buttons. | ||
*/ |
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 probably don't need this comment anymore
💚 Build SucceededBuild metrics@kbn/optimizer bundle module count
async chunks size
page load bundle size
History
To update your PR or re-run it, just comment with: |
…nesting warning (elastic#74499) Refactors the action buttons for transforms and analytics job list to no longer produce nested button elements and throw a React error.
* master: (30 commits) [code coverage] always download node before team assignment (elastic#75424) [Form lib] Allow new "defaultValue" to be provided when resetting the… (elastic#75302) [Logs UI] Add "View in machine learning" links in the anomaly explorer (elastic#74555) skip flaky suite (elastic#75440) skip flaky suite (elastic#75386) [Saved objects] Add support for version on create & bulkCreate when overwriting a document (elastic#75172) [Functional]Table Vis increase sleep time in order filter to be applied (elastic#75138) MOAR RAM (elastic#75423) [Visualize] Horizontal Bar Percentiles Overlapping (elastic#75315) [ML] DF Analytics / Transforms: Fix job row actions menu invalid DOM nesting warning (elastic#74499) [ML] Inference models management (elastic#74978) [Monitoring] Migrate karma tests (elastic#75301) [Index template] Add filters to simulate preview (elastic#74497) Bump and consolidate dependencies (elastic#75360) [Ingest Manager] Fix agent config rollout rate limit to use constants (elastic#75364) Update Node.js to version 10.22.0 (elastic#75254) [ML] Anomaly Explorer / Single Metric Viewer: Fix error reporting for annotations. (elastic#74953) [Discover] Fix histogram cloud tests (elastic#75268) Uiactions to navigate to visualize or maps (elastic#74121) Use prefix search invis editor field/agg combo box (elastic#75290) ...
Summary
Fixes #63455.
Refactors the action buttons for transforms and analytics job list to no longer produce nested button elements and throw a React error.
Checklist
For maintainers