Skip to content
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] Migrate Anomaly Explorer to React (except job selector) #28234

Merged
merged 29 commits into from
Jan 11, 2019

Conversation

walterra
Copy link
Contributor

@walterra walterra commented Jan 8, 2019

Summary

Migrates most parts of Anomaly Explorer to React.

  • explorer.html is mostly eliminated. It now only contains the job selector and filter bar (which is not in use). Instead of the previous angular code, the template now includes <ml-explorer-react-wrapper />.
  • <ml-explorer-react-wrapper /> is a directive that inherits the scope of explorer_controller.js. Once all of explorer.html is migrated, this directive can go away again. The directive listens for scope changes in explorer_controller.js and passes on the required scope attributes as props to <Explorer /> (explorer.js).
  • Some parts have been more "properly" converted than others. For example, messages for no existing jobs or data have been converted using EuiEmptyPrompt, angular code which used plain EUI classes now uses EUI component like EuiFlexGroup etc. This was done for parts which were straight-forward to migrate without touching too much application logic and risk of breaking things.
  • Some layout is still backed by bootstrap classes, esp. the main layout of the influencers and main column. This will be done in a follow-up.
  • Some middle-man directives which were wrappers for React components only are finally gone (e.g. influencers_list_directive.js). Some of these included logic to transform scope to props which is now part of explorer.js.
  • This PR cleans up and simplifies some application logic (esp. gets rid of some event listener/services), but there's room for more improvements in follow ups. The main aim of this PR is to get rid of most of the angularjs template explorer.html.
  • Some jquery is still used related to tooltips and to determine the width for explorer charts. This was already part of the previously migrated React components and has been a bit refactored in this PR to make it still work with the <Explorer /> component. We should soon get rid of this completely.

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers


Part of #18374.

@walterra walterra added non-issue Indicates to automation that a pull request should not appear in the release notes v7.0.0 :ml refactoring Feature:ml-results legacy - do not use v6.7.0 labels Jan 8, 2019
@walterra walterra self-assigned this Jan 8, 2019
@walterra walterra requested a review from a team as a code owner January 8, 2019 09:44
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui

@walterra walterra added the WIP Work in progress label Jan 8, 2019
@walterra walterra changed the title [WIP] [ML] Migrate Anomaly Explorer to React (expect job selector) [WIP] [ML] Migrate Anomaly Explorer to React (except job selector) Jan 8, 2019
@elasticmachine
Copy link
Contributor

💔 Build Failed

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes to SASS are approved ;)

@walterra walterra force-pushed the ml-explorer-react branch 2 times, most recently from 098a1ee to f0fff26 Compare January 9, 2019 10:06
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@walterra
Copy link
Contributor Author

retest

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of comments, but otherwise LGTM,

Copy link
Contributor

@alvarezmelissa87 alvarezmelissa87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM ⚡️

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@walterra walterra merged commit c58c357 into elastic:master Jan 11, 2019
@walterra walterra deleted the ml-explorer-react branch January 11, 2019 09:35
walterra added a commit to walterra/kibana that referenced this pull request Jan 11, 2019
…#28234)

* [ML] Move Anomaly Explorer Loading indicator to React.
* [ML] Move no-jobs message to React/Eui.
* [ML] Move no-results message to React/Eui.
* [ML] Refactored explorer.js to return earlier.
* [ML] Refactored influencers column to react.
* [ML] Refactored Overall Swimlane and view-by dropdown to react/eui.
* [ML] Refactored limit dropdown to react/eui.
* [ML] Refactored view-by swimlanes to React/Eui.
* [ML] Refactored annotations table to React/Eui.
* [ML] Refactored table controls to React/Eui.
* [ML] Refactored explorer charts to use React/Eui.
* [ML] Refactored anomalies table to React/Eui.
* [ML] Move explorer charts data listener to ExplorerChartsContainer component.
* [ML] Make AppState dependent services importable by React components.
* [ML] Removes deprecated code.
* [ML] Simplify state handling for anomaly charts.
* [ML] Simplify swimlaneCellClick().
* [ML] Review feedback: Fix file structure, add propTypes.
* [ML] Review feedback: Avoid anonymous inline functions.
* [ML] Fixes tests to reflect code changes.
* [ML] Fixes InfluencerList DOM position.
* [ML] Show a loading indicator when the view-by swimlane updates.
* [ML] Review feedback: Import only relevant lodash bits. Use querySelector instead of jQuery.
* [ML] Adds snapshot tests for new smallish components.
* [ML] Fix test stub.
* [ML] More resilient getChartContainerWidth().
* [ML] Review feedback: Comment on legacy utils and dropdown widths.
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

walterra added a commit that referenced this pull request Jan 11, 2019
…#28558)

* [ML] Move Anomaly Explorer Loading indicator to React.
* [ML] Move no-jobs message to React/Eui.
* [ML] Move no-results message to React/Eui.
* [ML] Refactored explorer.js to return earlier.
* [ML] Refactored influencers column to react.
* [ML] Refactored Overall Swimlane and view-by dropdown to react/eui.
* [ML] Refactored limit dropdown to react/eui.
* [ML] Refactored view-by swimlanes to React/Eui.
* [ML] Refactored annotations table to React/Eui.
* [ML] Refactored table controls to React/Eui.
* [ML] Refactored explorer charts to use React/Eui.
* [ML] Refactored anomalies table to React/Eui.
* [ML] Move explorer charts data listener to ExplorerChartsContainer component.
* [ML] Make AppState dependent services importable by React components.
* [ML] Removes deprecated code.
* [ML] Simplify state handling for anomaly charts.
* [ML] Simplify swimlaneCellClick().
* [ML] Review feedback: Fix file structure, add propTypes.
* [ML] Review feedback: Avoid anonymous inline functions.
* [ML] Fixes tests to reflect code changes.
* [ML] Fixes InfluencerList DOM position.
* [ML] Show a loading indicator when the view-by swimlane updates.
* [ML] Review feedback: Import only relevant lodash bits. Use querySelector instead of jQuery.
* [ML] Adds snapshot tests for new smallish components.
* [ML] Fix test stub.
* [ML] More resilient getChartContainerWidth().
* [ML] Review feedback: Comment on legacy utils and dropdown widths.
@sophiec20 sophiec20 added the Feature:Anomaly Detection ML anomaly detection label Jun 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Anomaly Detection ML anomaly detection Feature:ml-results legacy - do not use :ml non-issue Indicates to automation that a pull request should not appear in the release notes refactoring v6.7.0 v7.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants