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] Delete old AngularJS data visualizer and refactor folders #42962

Merged

Conversation

peteharverson
Copy link
Contributor

@peteharverson peteharverson commented Aug 8, 2019

Summary

Follow-up to #42685, removing the old AngularJS based data visualizer and field data card component.

Also restructures the datavisualizer directory to have a sub-directory for the index-based data visualizer, and one for the file-based data visualizer, and moves the selector page to the root of this folder:

datavisualizer
  file_based/
  index_based/
  datavisualizer_selector.js
  directive.js
  index.js

Also includes fix for file data visualizer file picker style, which was originally included in #42926.

Checklist

For maintainers

  • [ ] This includes a feature addition or change that requires a release note and was labeled appropriately

@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

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.

Tested this locally and LGMT ⚡️

Copy link
Contributor

@walterra walterra left a comment

Choose a reason for hiding this comment

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

Added a question about a new route name which is unclear to me

import { getDataVisualizerBreadcrumbs } from './breadcrumbs';

const template = `<ml-nav-menu name="datavisualizer" /><ml-data-visualizer />`;

uiRoutes.when('/data_visualizer', {
uiRoutes.when('/jobs/new_job/datavisualizer', {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain why this route is now prefixed with /jobs/new_job/? Do we maybe need to consider a redirect to support bookmarks to the old route?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The route /jobs/new_job/datavisualizer is unchanged - it used to be defined in datavisualizer_controller.js for the old AngularJS page. It contains jobs/new_job as the page was originally only accessed from the ''Create new job' page, where it lists the types of job wizards available and also a link to the Data Visualizer.

The directive for the selector page in the top-level folder contains the datavisualizer route. /data_visualizer was only there temporarily whilst I had links for the old and new data visualizer pages.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for clarifying!

Copy link
Contributor

@walterra walterra left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@jgowdyelastic jgowdyelastic 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 Succeeded

@peteharverson peteharverson merged commit f55e6da into elastic:master Aug 9, 2019
@peteharverson peteharverson deleted the ml-data-visualizer-folders branch August 9, 2019 11:02
peteharverson added a commit to peteharverson/kibana that referenced this pull request Aug 9, 2019
…ic#42962)

* [ML] Delete old AngularJS data visualizer - Resolve merge conflicts

* [ML] Fix imports

* [ML] Updating translation files
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

peteharverson added a commit that referenced this pull request Aug 9, 2019
… (#43015)

* [ML] Delete old AngularJS data visualizer - Resolve merge conflicts

* [ML] Fix imports

* [ML] Updating translation files
jloleysens added a commit to jloleysens/kibana that referenced this pull request Aug 9, 2019
…p-metrics-selectall

* 'master' of github.com:elastic/kibana: (306 commits)
  [ML] Adding job overrides to the module setup endpoint (elastic#42946)
  [APM] Fix missing RUM url (elastic#42940)
  close socket timeouts without message (elastic#42456)
  Upgrade elastic/charts to 8.1.6 (elastic#42518)
  [ML] Delete old AngularJS data visualizer and refactor folders (elastic#42962)
  Add custom formatting for Date Nanos Format (elastic#42445)
  [Vega] Shim new platform - vega_fn.js -> vega_fn.js , use ExpressionFunction (elastic#42582)
  add socket.getPeerCertificate to KibanaRequest (elastic#42929)
  [Automation] ISTANBUL PRESET PATH is not working fine with constructor(private foo) (elastic#42683)
  [ML] Data frames: Updated stats structure. (elastic#42923)
  [Code] fixed the issue that the repository can not be deleted in some cases. (elastic#42841)
  [kbn-es] Support for passing regex value to ES (elastic#42651)
  Connect to Elasticsearch via SSL when starting kibana with `--ssl` (elastic#42840)
  Add Elasticsearch SSL support for integration tests (elastic#41765)
  Fix duplicate fetch in Visualize (elastic#41204)
  [DOCS] TSVB and Timelion clean up (elastic#42953)
  [Maps] [File upload] Fix maps geojson upload hanging on index step (elastic#42623)
  [APM] Use rounded bucket sizes for transaction distribution (elastic#42830)
  [yarn.lock] consistent resolve domain (elastic#42969)
  [Uptime] [Test] Repurpose unit test assertions to avoid flakiness (elastic#40650)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:ml refactoring release_note:skip Skip the PR/issue when compiling release notes review v7.4.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants