-
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
Migrate timelion to the NP. #69160
Migrate timelion to the NP. #69160
Conversation
@elasticmachine merge upstream |
@elasticmachine merge upstream |
src/optimize/base_optimizer.js
Outdated
@@ -270,7 +270,6 @@ export default class BaseOptimizer { | |||
name: 'commons', | |||
chunks: (chunk) => | |||
chunk.canBeInitial() && chunk.name !== 'light_theme' && chunk.name !== 'dark_theme', | |||
minChunks: 2, |
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.
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.
@spalger Could you please take a look? We need your eyes here:)
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 like this is outdated now (sorry, was on vacation last week)
Pinging @elastic/kibana-app (Team:KibanaApp) |
#68284 touches the same thing - once it's merged it should fix this problem as well. |
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.
SASS files were simply moved. 👍
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.
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">"
@elasticmachine merge upstream |
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.
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.
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! Tested locally
@elasticmachine merge upstream |
@alexwizp Seems like there is a
|
@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 |
@elasticmachine merge upstream |
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 noticed two other things that don't work, but they are easy to fix:
- When in the app, clicking the navlink doesn't take you back to a new sheet, instead nothing happens. This is the case because the URL change is not picked up by the hash history object. Visualize fixes this with the following trick: https://github.com/flash1293/kibana/blob/7c352c0702d35f3e68451936dfc7c679dd67e8e1/src/plugins/visualize/public/plugin.ts#L135 We can use the same approach in timelion
- The sub-url tracker doesn't work - when switching from Timelion to Discover and back, the local state is lost. This is the case because a hash history object instead of the platform scoped history object is used. Again we can use the same fix as in visualize: https://github.com/flash1293/kibana/blob/7c352c0702d35f3e68451936dfc7c679dd67e8e1/src/plugins/visualize/public/plugin.ts#L106
Other than those, this looks good to me.
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 in Firefox and LGTM once green. Thanks @VladLasitsa for powering through this!
💚 Build SucceededBuild metrics@kbn/optimizer bundle module count
History
To update your PR or re-run it, just comment with: |
* 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>
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. |
closes #67468
Summary
Migrated timelion to the new platform.
Checklist
Delete any items that are not applicable to this PR.
For maintainers