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

Migrate timelion to the NP. #69160

Merged
merged 29 commits into from
Jul 14, 2020
Merged

Migrate timelion to the NP. #69160

merged 29 commits into from
Jul 14, 2020

Conversation

VladLasitsa
Copy link
Contributor

closes #67468

Summary

Migrated timelion to the new platform.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@VladLasitsa VladLasitsa self-assigned this Jun 15, 2020
@VladLasitsa
Copy link
Contributor Author

@elasticmachine merge upstream

@VladLasitsa
Copy link
Contributor Author

@elasticmachine merge upstream

@VladLasitsa VladLasitsa marked this pull request as ready for review June 23, 2020 09:21
@VladLasitsa VladLasitsa requested a review from a team June 23, 2020 09:21
@VladLasitsa VladLasitsa requested review from a team as code owners June 23, 2020 09:21
@VladLasitsa VladLasitsa added Feature:NP Migration release_note:skip Skip the PR/issue when compiling release notes v7.9.0 v8.0.0 labels Jun 23, 2020
@@ -270,7 +270,6 @@ export default class BaseOptimizer {
name: 'commons',
chunks: (chunk) =>
chunk.canBeInitial() && chunk.name !== 'light_theme' && chunk.name !== 'dark_theme',
minChunks: 2,
Copy link
Contributor Author

@VladLasitsa VladLasitsa Jun 23, 2020

Choose a reason for hiding this comment

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

After migration "timeloin" commons.styles.css is no longer loaded. Because of this bootstrap.js went to 'exception' path and I got an error related to redirect from old URL to new, also font_awesome styles didn't load. I deleted minСhunks property so that commons.styles can be loaded.
Capture

@spalger Could you please take a look? We need your eyes here:)

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this is outdated now (sorry, was on vacation last week)

@VladLasitsa VladLasitsa added the Team:Visualizations Visualization editors, elastic-charts and infrastructure label Jun 23, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

@flash1293
Copy link
Contributor

flash1293 commented Jun 23, 2020

#68284 touches the same thing - once it's merged it should fix this problem as well.

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.

SASS files were simply moved. 👍

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Noticed a few things:

  • You have to resolve a few conflicts because of another PR I merged in. Make sure to carry over this line: https://github.com/elastic/kibana/blob/master/src/legacy/core_plugins/timelion/public/plugin.ts#L70 It makes sure font-awesome is loaded correctly (you have to depend on kibanaLegacy to do so).
  • It's also broken in 7.8, but the "Refresh" button doesn't do anything. Could you have a look while you are at it?
  • When I'm using the "Save current expression as Kibana dashboard panel" function, it will create a visualization saved object, but opening it in Visualize will break the editor and render nothing (see error below). This works in 7.8
react_devtools_backend.js:6 TypeError: Cannot read property 'getOwnField' of undefined
    at visStateToEditorState (editor.js:121)
    at VisualizeAppController (editor.js:130)
    at Object.invoke (angular.js:5143)
    at $controllerInit (angular.js:11707)
    at nodeLinkFn (angular.js:10520)
    at compositeLinkFn (angular.js:9835)
    at publicLinkFn (angular.js:9700)
    at Object.link (angular-route.js:1260)
    at angular.js:1390
    at invokeLinkFn (angular.js:11269) "<div ng-view="" class="visAppWrapper ng-scope">"
  • Somehow panels can't be reordered by dragging anymore (works in 7.8)
    reordering
  • Deleting panels doesn't work anymore - it will delete a panel but not the right one and afterwards the expressions in the editor don't match the selected panel
    deleting

@VladLasitsa
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

It seems like timelion is not torn down correctly when navigating away - When switching from Home -> Timelion -> Discover -> Timelion it won't render and all subsequent navigations to other apps won't work as well.

The rest works fine, great work there! Saving a single panel also seems to work perfectly now.

Copy link
Contributor

@alexwizp alexwizp left a comment

Choose a reason for hiding this comment

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

LGTM! Tested locally

@alexwizp
Copy link
Contributor

@elasticmachine merge upstream

@flash1293
Copy link
Contributor

@alexwizp Seems like there is a requiredBundles missing:

ERROR in ./public/directives/saved_object_finder.js
10:33:54         │          Module not found: Error: import [../../../saved_objects/public] references a public export of the [savedObjects] bundle, but that bundle is not in the "requiredPlugins" or "requiredBundles" list in the plugin manifest [/dev/shm/workspace/kibana/src/plugins/timelion/kibana.json]
1

@flash1293
Copy link
Contributor

@alexwizp I'm not sure what caused this (I'm 85% certain it worked in an earlier iteration of this PR), but when editing the. expression, it's not possible to select an entry from the suggestion box via "arrow down" key anymore and "cmd+enter" doesn't execute the expression. Both things work in 7.8

@alexwizp
Copy link
Contributor

@elasticmachine merge upstream

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

I noticed two other things that don't work, but they are easy to fix:

Other than those, this looks good to me.

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Tested in Firefox and LGTM once green. Thanks @VladLasitsa for powering through this!

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

id value diff baseline
timelion 59 +59 -

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@VladLasitsa VladLasitsa merged commit d8823d8 into elastic:master Jul 14, 2020
VladLasitsa added a commit to VladLasitsa/kibana that referenced this pull request Jul 14, 2020
* Migrate timelion to the NP.

* fixed ci

* Fixed paths

* fixed UI settings

* Fixed ci

* fix CI

* Fixed some comments

* Fixed browser tests

* fixed state

* Fixed comments

* Fixed save expression

* Fixed navigation

* fix CI

* Fixed some problem

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Co-authored-by: Alexey Antonov <alexwizp@gmail.com>
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jul 16, 2020
@timroes timroes added v7.10.0 and removed v7.9.0 labels Jul 16, 2020
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jul 17, 2020
flash1293 pushed a commit that referenced this pull request Jul 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:NP Migration release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate timelion application
8 participants