-
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
Remove legacy data table #111339
Remove legacy data table #111339
Conversation
@@ -13,7 +13,6 @@ const alwaysImportedTests = [ | |||
require.resolve('../test/ui_capabilities/newsfeed_err/config.ts'), | |||
require.resolve('../test/new_visualize_flow/config.ts'), | |||
require.resolve('../test/security_functional/config.ts'), | |||
require.resolve('../test/functional/config.legacy.ts'), |
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.
ℹ️ This functional test suite runs the tests for the legacy implementation (angular). The same tests run for the new implementation so we don't lose our test coverage here!
@elasticmachine merge upstream |
Pinging @elastic/kibana-vis-editors (Team:VisEditors) |
@elasticmachine merge upstream |
@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.
Code review 👍
Did some testing with Safari and Chrome, could not find any issue with that.
Confirm the kibana.yml
setting has been correctly removed.
Should we actually remove the yml setting or just deprecate it for the PR that will go in 7.16 (which will log a warning)? Otherwise this will introduce a breaking change preventing Kibana server startup in 7.16 which can be avoided. For 8.0 I agree we should remove it completely. |
Good point. I've just discussed with @stratoula and she mentioned the behaviour was expected and the core team was aware of it, but migration-wise it can be improved, I agree. |
Discussed offline with the team. We decided to merge it as it is and re-discuss it on our weekly sync. |
* [Datatable] Removes the angular implementation * Fix i18n errors * Fix types * Remove unecessary funtional tests * Fix documentation Co-authored-by: Kibana Machine <[email protected]> # Conflicts: # scripts/functional_tests.js
* [Datatable] Removes the old implementation (#111339) * [Datatable] Removes the angular implementation * Fix i18n errors * Fix types * Remove unecessary funtional tests * Fix documentation Co-authored-by: Kibana Machine <[email protected]> # Conflicts: # scripts/functional_tests.js * Fix conflict Co-authored-by: Kibana Machine <[email protected]>
💔 Build Failed
Failed CI Steps
Test FailuresKibana Pipeline / general / task-queue-process-9 / X-Pack Endpoint API Integration Tests.x-pack/test/security_solution_endpoint_api_int/apis/package·ts.Endpoint plugin Endpoint package geoip processor sets the geoip fieldsStandard Out
Stack Trace
Kibana Pipeline / general / Chrome UI Functional Tests.test/functional/apps/visualize/_tag_cloud·ts.visualize app visualize ciGroup12 tag cloud chart should collapse the sidebarStandard Out
Stack Trace
Kibana Pipeline / general / Chrome UI Functional Tests.test/functional/apps/dashboard/dashboard_time_picker·ts.dashboard app using legacy data dashboard time picker Visualization updated when time picker changesStandard Out
Stack Trace
and 24 more failures, only showing the first 3. Metrics [docs]Module Count
Async chunks
Page load bundle
Unknown metric groupsasync chunk count
References to deprecated APIs
History
To update your PR or re-run it, just comment with: |
Hey team - while testing this PR in BC3 of 7.16.0 - I added in vis_type_table.legacyVisEnabled: true in kibana.yml and I see this in the logs -
Is this working as intended? Thanks! |
Yes - the legacy table implementation is gone, so you don't need the setting anymore. You have to use the new table implementation in 7.16 |
@flash1293 I got that part but got confused about kibana.yml settings after reading the comments. Thank you! |
Summary
Closes #90582
Part of #109382
Closes #104806
This PR removes the legacy implementation of the datatable visualization (which uses angular). This means that the kibana.yml setting
vis_type_table.legacyVisEnabled
has also been removed.Release note
We removed the legacy implementation of the aggregation-based datatable. In order to auto fit the height of table rows to its content, please enable the "Auto fit rows to content" setting in the "Options" tab of each table visualization (#114637).