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

[Discover] EuiDataGrid Implementation #67259

Merged
merged 272 commits into from
Dec 23, 2020

Conversation

kertal
Copy link
Member

@kertal kertal commented May 25, 2020

Summary

Continuation of #51531 🔥 integrating EuiDataGrid implementation in Discover including the saved search embeddable.

You need to enable it in Stack Monitoring / Advanced Settings by setting Use legacy table to Off

Bildschirmfoto 2020-12-07 um 17 09 09

When you navigate to Discover you should see the EuiDataGrid in Action

Bildschirmfoto 2020-12-07 um 17 14 15

You can sort columns now with drag and drop

image

View the grid in full screen mode
image

Enjoy a much better way to change sorting

image

Expand documents in a flyout

image

You can resize one or multiple column, this is persisted in the saved search saved object and when you embed a saved search in a dashboard the width

Resolves #737
Resolves #38982
Resolves #8706
Resolved #4436

Known bugs and issues

  • EuiDataGrid - Wrong columns assigned to leadColumns (PR), so cell actions are displayed e.g. in the expand section of grid
  • EuiDataGrid Display sort arrow next in header column [EuiDataGrid] Add sorting icon if the column is sorted eui#4343 (comment)
  • EuiDataGrid - Cell actions spacing the Safari (issue)
  • EuiDataGrid - Resize handler overlapping header menu button at the rightmost column (issue)
  • EuiDataGrid - Cell actions need 2 clicks in Firefox and Safari on MacOS (issue)
  • EuiDataGrid - Cellpopover stays open even underlying data changes (issue)

Follow ups

Checklist

@kertal kertal self-assigned this May 25, 2020
@kertal kertal added the Feature:Discover Discover Application label May 25, 2020
@kertal
Copy link
Member Author

kertal commented May 27, 2020

FYI: @myasonik & @snide

@kertal
Copy link
Member Author

kertal commented Dec 18, 2020

Some notes.

  • We need to fix the spacing in the blob view. Right now there are no margins to the left of the blocks.
  • Selected state of rows needs emphasis

you took care of that, right @cchaos

  • We should add a toast or something to signify the actions in the flyouts happened. Since they are hidden behind.

Or we make them function like the legacy one, close the flyout, would also prevent a problem with toggling of columns

  • Products field is rendering empty in the grid, but shows in the flyout.

Muchas gracias, fixed that

  • In general, I think we need a schema to recognize JSON blobs, and apply that schema from EUI (for syntax highlighting in the popout)
  • We can set the height for the chart header so that there's not a jump as you hide the chart. (this is because the select form has a larger height)

Dankeschön for taking care of it

  • I would increase the default pagination to more than 25 results. It should be as high as we feel comfortable.

Will be done when virtualization, I'm comfortable with 5000000 then

about the geo field schema, so it should be handled like a JSON or differently? thx

@snide
Copy link
Contributor

snide commented Dec 18, 2020

@kertal did some more styling. We have some Amsterdam specific changes that we'll need to make (that shouldn't hold up this PR) after the break.

I'd still like to highlight the row selected if possible. Think you can pass me a className on the cells for the selected row? That way I can give it some emphasis once it's been expanded.

Other small thing I noticed in here (and can wait) is that we really need to replace the doc viewer table that shows in the expanded flyout to use an EuiTable. Right now it's using Bootstrap, and we're looking to remove that next minor.

Copy link
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

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

This looks great. There are some small leftover issues from design, but they can all be handled by later PRs. All the big stuff is covered. When we get closer to a release, we should make a more prominent switch (maybe a one time toast?) to let people toggle the new view, that way they don't need to dig into adv. settings.

There's likely nearly ten people on this PR on the EUI or Kibana side. Really nice job by everyone. Good to merge!

@kertal kertal merged commit a417690 into elastic:master Dec 23, 2020
majagrubic pushed a commit that referenced this pull request Dec 23, 2020
Co-authored-by: Michail Yasonik <[email protected]>
Co-authored-by: Marta Bondyra <[email protected]>
Co-authored-by: Dave Snider <[email protected]>
Co-authored-by: Andrea Del Rio <[email protected]>
Co-authored-by: cchaos <[email protected]>

Co-authored-by: Matthias Wilhelm <[email protected]>
Co-authored-by: Michail Yasonik <[email protected]>
Co-authored-by: Marta Bondyra <[email protected]>
Co-authored-by: Dave Snider <[email protected]>
Co-authored-by: Andrea Del Rio <[email protected]>
Co-authored-by: cchaos <[email protected]>
@kibanamachine
Copy link
Contributor

kibanamachine commented Apr 20, 2021

💔 Build Failed

Failed CI Steps

Metrics [docs]

‼️ ERROR: no builds found for mergeBase sha [a98550b]

History

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Discover Discover Application release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.12.0 v8.0.0
Projects
None yet