-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Discover] Enable EuiDataGrid as default document table #89264
[Discover] Enable EuiDataGrid as default document table #89264
Conversation
@elasticmachine merge upstream |
10 days ago no test failed ... well, some cleanup work ahead |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
…-26-discover-enable-data-grid
…ub.com:kertal/kibana into kertal-pr-2020-01-26-discover-enable-data-grid
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 only - covered the dashboard functional test updates. Presentation team changes LGTM.
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.
In a previous version of this the page sizes got increased (50,100,500 or something like this). This PR leaves them as 25/50/100, is this on purpose?
I'm not sure whether the EUI update fixing this went in already, but changing the page size too 100 is still relatively slow for me - not a blocker, it works fine, but takes a few seconds.
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.
Approving as I think we can merge this for 7.13
…-26-discover-enable-data-grid
…-26-discover-enable-data-grid
Yes, because the EUI update with fixes is incoming: That's why I didn't increase numbers
What takes a few seconds? I've tested in locally and changing pagination size was fast? thx! |
Changing from 25 to 100 rows with a full 500 docs loaded. But if the fix for this on EUI side isn't in yet, that would explain it. I think we can move forward with this PR> |
@elasticmachine merge upstream |
* Now the new EuiDataGrid based document table is the default * Columns can be sorted by drag and drop * Column width can be changed by drag and drop * There's a fullscreen mode * There's document selection * There's document navigation in the flyover of a expanded document * Sorting is much more user friendly, less confusing and sort order can be changed by drag and drop # Conflicts: # test/functional/apps/discover/_discover.ts # x-pack/test/functional/apps/security/doc_level_security_roles.js # x-pack/test/functional/apps/security/field_level_security.js
* Now the new EuiDataGrid based document table is the default * Columns can be sorted by drag and drop * Column width can be changed by drag and drop * There's a fullscreen mode * There's document selection * There's document navigation in the flyover of a expanded document * Sorting is much more user friendly, less confusing and sort order can be changed by drag and drop
💚 Build SucceededMetrics [docs]
History
To update your PR or re-run it, just comment with: cc @kertal |
* Now the new EuiDataGrid based document table is the default * Columns can be sorted by drag and drop * Column width can be changed by drag and drop * There's a fullscreen mode * There's document selection * There's document navigation in the flyover of a expanded document * Sorting is much more user friendly, less confusing and sort order can be changed by drag and drop
Summary
This PR enables the new EuiDataGrid in Discover as a default grid in Discover and Dashboard saved searches.
Here’s a quick summary of the improvements:
Performance (Outdated part)
With the merge of #89648 performance, the biggest pain point will be fine, in my latest testing (production build version of #89648), legacy grid mounting was very slow, so I will search for a recent regression here.So in my latest test I was rendering Discover in Chrome on a MacBookPro 2018. Been using the browser's performance tools to compare the time for initial rendering, adding columns, expanding documents. That was the setup:
And those are the numbers how long rendering takes until the user sees a result:
It's no big surprise that "Expanding a doc" is slower now, because there's a flyout displayed. Numbers may vary depending on screen size and setup, but all in all I can say: performance is fine. Will retry next week the #89648 was merged, have a look at angular world, and there's another performance improving PR in the pipeline, which should reduce re-renderings #89543.
Performance (latest, using #97276)
Tests using Chrome performance tools, start of scripting until the grid + data is displayed, settings pagination to 100
Summary: In the given test EuiDataGrid is a bit faster than the legacy grid, except when expanding a document, here is further potential to improve.
Fixes #12417
Checklist