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

[Data table] Reactify & EUIficate the visualization #70801

Merged
merged 103 commits into from
Dec 15, 2020

Conversation

sulemanof
Copy link
Contributor

@sulemanof sulemanof commented Jul 6, 2020

Summary

Resolves #64444
Resolves #2516
Resolves #2128

This converts the Data table visualization into React + EUI DataGrid component.

By default a new version of visualization will be used. Discussed with @timroes .
To turn on the legacy one, include vis_type_table.legacyVisEnabled: true setting in kibana.yml.

Single table mode:

image

Split table mode (several grids displayed):

image

The split table mode could have performance issue while a lot of data grids render at once.
Despite this, the PR is good to go, since:

  1. the feature is not widely used;
  2. the legacy vis also has the inherent performance issue;

A separate issue is created to track and potentially solve the problem: #84509


Changes include:


Release notes:

  • Show toolbar option adds an ability to show/hide the visualization toolbar, where the only Export button currently live (the toolbar will be expanded in future releases).
    By default, a new visualization will have the toolbar disabled. Saved objects will be migrated to have the toolbar enabled initially.
    ( feature commit, migration script commit )

    export_table

  • Exported as CSV tables will preserve the sorting by column direction!

  • Customizing a column width option is in. The value is saved into the uiState - that means columns width will be saved even after page reload. While using Split table mode, columns width will be synchronized between tables.

    Single table mode
    column_resize

    Split table mode
    column_resize_split_table

  • Expand a cell to see a collapsed content - hover over the cell to see the expand button:

    expand_cell

Checklist

Delete any items that are not applicable to this PR.

For maintainers

sulemanof added 27 commits July 1, 2020 11:29
…table

# Conflicts:
#	src/plugins/vis_type_table/public/table_vis_type.ts
# Conflicts:
#	src/plugins/vis_type_table/public/agg_table/_agg_table.scss
#	src/plugins/vis_type_table/public/agg_table/agg_table.js
#	src/plugins/vis_type_table/public/paginated_table/rows.js
#	src/plugins/vis_type_table/public/vis_controller.ts
@sulemanof sulemanof added blocked WIP Work in progress Feature:Data Table Data table visualization feature labels Sep 22, 2020
@flash1293

This comment has been minimized.

@flash1293

This comment has been minimized.

@sulemanof

This comment has been minimized.

@sulemanof
Copy link
Contributor Author

@flash1293
the toolbar thing fixed in 9048a45 by adding the minSizeForControls prop.

@flash1293

This comment has been minimized.

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.

This LGTM

The following things are not working perfectly, but I would be fine with merging:

  • Weird behavior on row split: Resizing columns is very slow and has the chart moving for multiple seconds. Table are not always sized the same (sometimes very narrow, sometimes taking 100% of the width)
  • Column width / sorting not tied to column, but to index, causing some inconsistencies (e.g. not possible to reset column width to auto)
  • Some small EUI data grid problems (typings not matching, sorting popovers not closing) (edited)
  • Density settings has been removed

We still should decide on the name of the "toolbar flag" it's just controlling the csv export button now, but I don't think an additional round of review will be necessary

@stratoula

This comment has been minimized.

@cchaos

This comment has been minimized.

@stratoula

This comment has been minimized.

@stratoula

This comment has been minimized.

@sulemanof

This comment has been minimized.

Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

Thanx @sulemanof I tested it and it works now 🎉
Datatable seems to work as expected, I don't like a lot how it behaves on split tables but it seems fine for now 🙂 ++ for adding some unit tests ❤️

@sulemanof
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

app services code LGTM

@stratoula
Copy link
Contributor

@elasticmachine merge upstream

@stratoula stratoula dismissed LeeDr’s stale review December 15, 2020 08:11

The comments has been already addressed and other member of the team has already approved it

@sulemanof
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
visTypeTable 44 83 +39

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
visTypeTable 66.6KB 148.6KB +82.0KB

Distributable file count

id before after diff
default 47224 47994 +770
oss 27530 27540 +10

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
data 1000.9KB 1001.0KB +71.0B
visTypeTable 19.1KB 11.7KB -7.4KB
total -7.3KB
Unknown metric groups

async chunk count

id before after diff
visTypeTable 2 5 +3

History

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

@sulemanof sulemanof merged commit 845f716 into elastic:master Dec 15, 2020
sulemanof pushed a commit that referenced this pull request Dec 16, 2020
* Use data grid for table vis

* Create basic table template

* Add table_vis_split component

* Apply cell filtering

* Add aria-label attributes

* Use field formatters for values

* Add no results component

* Remove legacy dependencies

* Add usePagination

* Create usePagination util

* Use percentage column and total row

* Use csv export button

* Update labels

* Fix merge conflicts

* Use split table

* Fix functional tests

* Fix dashboard tests

* Update data table functional tests

* Fix missed test

* Introduce showToolbar option

* Remove useless package

* Fix merge conflicts

* Return back kibanaUtils required bundle

* Revert "Remove useless package"

This reverts commit 144a7cd.

* Use feature flag for legacy vis

* Add footer row

* Remove lock files

* Revert "Remove lock files"

This reverts commit 5c5acd7.

* Minor fixes

* Use common no result message

* Fix broken tests

* Use ui state sorting

* Fix error

* Fix merge conflicts

* Add legacy functional tests

* Pull pagination footer up to keep with table

and fix column split growing continuously in dashboard

* Comments fixes

* Use cell actions for filtering

* Fix translations

* Fix comments

* Reduce legacy tests amount

* Update sorting

* Update split column layout

* Add telemetry for legacy vis

* Apply latest changes for split table

* Fix eslint errors

* Use aria labels with values

* Use aria label for export btn

* Fix functional test

* Update translations

* Cleanup

* Truncate cells content

* Enhance types in table_vis_response_handler.ts

* Persist columns width on change

* Fix sorting history

* Add a migration script for toolbar

* Export sorted table

* Use reportUiCounter instead of reportUiStats

* Fix integration tests

* Fix typos

* Adjust FieldFormat type

* Hide the density selector

* Update docs

* Fix pagination

* Restrict hiding the toolbar

* Fix column index on filter

* Add closePopover action

Co-authored-by: cchaos <caroline.horn@elastic.co>
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
# Conflicts:
#	scripts/functional_tests.js

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Data Table Data table visualization feature release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.11.0 v8.0.0
Projects
None yet