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

[UnifiedDocViewer] Migrate table to EuiDataGrid #174745

Closed
Tracked by #163284
kertal opened this issue Jan 12, 2024 · 2 comments · Fixed by #175787
Closed
Tracked by #163284

[UnifiedDocViewer] Migrate table to EuiDataGrid #174745

kertal opened this issue Jan 12, 2024 · 2 comments · Fixed by #175787
Assignees
Labels
enhancement New value added to drive a business result Feature:Discover Discover Application Feature:UnifiedDocViewer Issues relating to the unified doc viewer component impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. loe:needs-research This issue requires some research before it can be worked on or estimated Team:DataDiscovery Discover App Team (Document Explorer, Saved Search, Surrounding documents, Graph)

Comments

@kertal
Copy link
Member

kertal commented Jan 12, 2024

To align with UnifiedDocViewer the table displaying fields and values should be migrated to make use of EuiDataGrid. While we were implementing float actions in #171365 we figured out, that implementing custom float action would create a custom pattern for handling action on a table. We aim to use consistent patterns for handling tables, and therefore we will move forward by using a different component providing table actions out of the box. This is also used in the main consumer of the UnifiedDocViewer, which is Discover. On top of this, it might also enable us to make use of features like configureable row height ,and cell width, which might be out of scope for the initial implementation ,but could be evaluated during implementation.

Before:

Bildschirmfoto 2024-01-12 um 12 33 55

After:

We should relevant actions per column:

  • For field: toggle field in column, pin and field exists
  • For value: filter for value, filter out value

Screen Recording 2024-01-12 at 9 07 57 AM

To be defined for a later phase

Should we include the data grid menus like we do in the main Discover view as well as the count of fields above the table. See:
image

@kertal kertal added the Feature:UnifiedDocViewer Issues relating to the unified doc viewer component label Jan 12, 2024
@botelastic botelastic bot added the needs-team Issues missing a team label label Jan 12, 2024
@kertal kertal added enhancement New value added to drive a business result and removed needs-team Issues missing a team label labels Jan 12, 2024
@botelastic botelastic bot added the needs-team Issues missing a team label label Jan 12, 2024
@kertal kertal added Feature:Discover Discover Application Team:DataDiscovery Discover App Team (Document Explorer, Saved Search, Surrounding documents, Graph) and removed needs-team Issues missing a team label labels Jan 12, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

@kertal kertal added impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. loe:needs-research This issue requires some research before it can be worked on or estimated labels Jan 12, 2024
@kertal
Copy link
Member Author

kertal commented Feb 2, 2024

@andreadelrio
Some feedback after testing #175787 and discussing in our sync:

  • The new design is appreciated also when working with the POC 👍
  • Performance impact doesn't seem to be significant, it looks worse in DevTools than in real life, we still need to do some benchmarking
  • There were concerns about the control change, generally we think it's better UX, and more aligned to the usage in the DataTable. What we could consider is to show a dedicated control columns when the screen size allows it? So we would keep the old behavior of showing all actions at one sight, while improving the usage of screen size on smaller screens by using EuiDataGrid
  • One other thing was asked, if we should consider aligning the scrolling behavior to the usage in Main Discover. So we scroll the body of the Grid and not the whole grid.
  • What might be benefical is to allow the user to configure the grid a bit, e.g. by letting users assign a different row height. We recently heard the complain, that with removing the collapse function for large text blobs, this can lead to more scrolling. Users can Pin fields, also search fields, which is helpful, but it's worth to consider, to let then decide about row height

jughosta added a commit that referenced this issue Jun 19, 2024
…00 fields per page. (#175787)

- Closes #174745

## Summary

This PR converts Doc Viewer table into EuiDataGrid to use its actions
functionality.

<img width="703" alt="Screenshot 2024-05-17 at 20 18 44"
src="https://github.com/elastic/kibana/assets/1415710/10d8a7b0-8fe1-4908-a11d-5fd374eed4c3">
<img width="577" alt="Screenshot 2024-05-17 at 18 17 49"
src="https://github.com/elastic/kibana/assets/1415710/7e6f05ce-9690-48ab-84c0-f8776e360f83">
<img width="490" alt="Screenshot 2024-05-17 at 18 18 05"
src="https://github.com/elastic/kibana/assets/1415710/b36c64de-419d-425c-9890-8bc346059c1a">
<img width="871" alt="Screenshot 2024-05-22 at 15 22 31"
src="https://github.com/elastic/kibana/assets/1415710/92c894f3-91f8-445c-b6fb-ba8842dd3b23">


## Testing

Some cases to check while testing:
- varios value formats
- legacy table vs data grid
- doc viewer flyout vs Single Document page

### Checklist

- [x] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [x] This renders correctly on smaller devices using a responsive
layout. (You can test this [in your
browser](https://www.browserstack.com/guide/responsive-testing-on-local-server))
- [x] This was checked for [cross-browser
compatibility](https://www.elastic.co/support/matrix#matrix_browsers)

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Andrea Del Rio <delrio.andre@gmail.com>
Co-authored-by: Davis McPhee <davismcphee@hotmail.com>
Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co>
Co-authored-by: Davis McPhee <davis.mcphee@elastic.co>
seanrathier pushed a commit to seanrathier/kibana that referenced this issue Jun 21, 2024
…00 fields per page. (elastic#175787)

- Closes elastic#174745

## Summary

This PR converts Doc Viewer table into EuiDataGrid to use its actions
functionality.

<img width="703" alt="Screenshot 2024-05-17 at 20 18 44"
src="https://github.com/elastic/kibana/assets/1415710/10d8a7b0-8fe1-4908-a11d-5fd374eed4c3">
<img width="577" alt="Screenshot 2024-05-17 at 18 17 49"
src="https://github.com/elastic/kibana/assets/1415710/7e6f05ce-9690-48ab-84c0-f8776e360f83">
<img width="490" alt="Screenshot 2024-05-17 at 18 18 05"
src="https://github.com/elastic/kibana/assets/1415710/b36c64de-419d-425c-9890-8bc346059c1a">
<img width="871" alt="Screenshot 2024-05-22 at 15 22 31"
src="https://github.com/elastic/kibana/assets/1415710/92c894f3-91f8-445c-b6fb-ba8842dd3b23">


## Testing

Some cases to check while testing:
- varios value formats
- legacy table vs data grid
- doc viewer flyout vs Single Document page

### Checklist

- [x] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [x] This renders correctly on smaller devices using a responsive
layout. (You can test this [in your
browser](https://www.browserstack.com/guide/responsive-testing-on-local-server))
- [x] This was checked for [cross-browser
compatibility](https://www.elastic.co/support/matrix#matrix_browsers)

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Andrea Del Rio <delrio.andre@gmail.com>
Co-authored-by: Davis McPhee <davismcphee@hotmail.com>
Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co>
Co-authored-by: Davis McPhee <davis.mcphee@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New value added to drive a business result Feature:Discover Discover Application Feature:UnifiedDocViewer Issues relating to the unified doc viewer component impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. loe:needs-research This issue requires some research before it can be worked on or estimated Team:DataDiscovery Discover App Team (Document Explorer, Saved Search, Surrounding documents, Graph)
Projects
None yet
3 participants