[Discover] Optimize popover size for grid cell expansion #152480

jughosta · 2023-03-01T16:55:20Z

Open to your feedback!

Summary

This PR makes changes to cell popover styles in Discover:

JSON view is larger by default and its size can be increased horizontally
Non-JSON view size can be increased horizontally and vertically

Gifs

Our JSON viewer is not very responsive according to its container dimensions so I was able to only allow horizontal resizing.

JSON popover height is equal to the max height for non-JSON popover.

~~Please note that current implementation would work only in new browsers. To address this, some styles should be moved to Eui or Eui cell popover should support custom classes.~~ elastic/eui#6632

Ideas for future improvements: currently it's not possible for users to reposition the cell popover. It might be better to later look into a different solution and open values in a flyout for example or as a modal.

jughosta · 2023-03-01T17:04:30Z

Hi @kertal,
I did some exploration here but it might be better to look into a different solution and open values in a flyout for example or as a modal. Or make that row expandable.

Wdyt? Should we enable resizing?

kertal · 2023-03-02T11:13:49Z

Hi @kertal, I did some exploration here but it might be better to look into a different solution and open values in a flyout for example or as a modal. Or make that row expandable.

Wdyt? Should we enable resizing?

Oh wow 😲 , I expected we could just configure width and height for the grid consumers, and you actually implemented ... resizing:

grid-resizing.mp4

I think this would definitely be valuable, but it also adds more questions and tasks, and it doesn't sound like a small task. For instance the configured size should be remembered.

Please note that current implementation would work only in new browsers. To address this, some styles should be moved to Eui or Eui cell popover should support custom classes.

What are the browser requirements? Also looping in @elastic/eui-design if there are any plans in this area we are not aware of

jughosta · 2023-03-02T12:18:57Z

What are the browser requirements?

@kertal I am using :has CSS selector here to temporary define custom styles for .euiDataGridRowCell__popover. It's not supported everywhere yet. Ideally, similar styles would be implemented on Eui side - no need to override in Kibana.

cee-chen · 2023-03-02T20:33:44Z

What are the browser requirements? Also looping in https://github.com/orgs/elastic/teams/eui-design if there are any plans in this area we are not aware of

Ideally, similar styles would be implemented on Eui side - no need to override in Kibana.

No current plans, but it sounds like y'all need some way of setting props (custom classNames or what have you) on the rendered cell popover. Would us passing a function like setCellPopoverProps (same or similar API as setCellProps) be useful to you all? That way you can apply your custom sizing/resizing CSS without needing :has.

jughosta · 2023-03-06T11:54:16Z

@cee-chen Yes, setCellPopoverProps would be great!

cee-chen · 2023-03-06T17:34:25Z

elastic/eui#6632 - likely will be a bit longer until it lands in Kibana though as a heads up - maybe another week or two!

…pover

elasticmachine · 2023-04-05T16:51:40Z

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

davismcphee

Cool idea and it seems to be working well overall! Although I think it's something we should ask for design input on before we settle on a solution.

A couple of notes about the current implementation:

It seems like the JSON popover is constrained to the screen width, but I can resize the plain text popover off the screen so it can't be resized again without closing and reopening. Ideally both popovers would be constrained to the screen size if possible.
It would be best if we could get the vertical resizing working with the JSON popover since it would probably benefit most from this functionality anyway. I see that the JSON viewer takes an explicit height as a prop, but I was able to hack together a somewhat working solution in a few minutes using a ResizeObserver that manually updates the height when resized, so maybe that would be an option to explore.

davismcphee · 2023-04-06T03:14:17Z

src/plugins/discover/public/components/discover_grid/discover_grid.tsx

@@ -380,6 +381,8 @@ export const DiscoverGrid = ({
    [dataView, displayedRows, useNewFieldsApi, shouldShowFieldHandler, services.uiSettings]
  );

+  const renderCellPopover = useMemo(() => getRenderCellPopoverFn(), []);


Nit: it seems like it would be simpler to pass in something like a CellPopover component directly instead of wrapping it with getRenderCellPopoverFn since we're not using the closure for anything.

kertal · 2023-04-12T10:02:08Z

src/plugins/discover/public/components/discover_grid/discover_grid.scss

+  &.dscDiscoverGrid__cellPopover--withJson {
+    resize: horizontal;
+    min-width: 516px;
+    width: 516px;
+  }


I definitely think, even if we decide not to allow resizing, having the JSON view in a larger dimension makes much sense

kertal · 2023-04-12T10:10:55Z

Cool idea and it seems to be working well overall! Although I think it's something we should ask for design input on before we settle on a solution.

+1 from my side, @andreadelrio can you have a look? link

I guess it would be more effort, but to make this actually useful, at least we should have some sort of memory for the user assigned dimensions. So when the user scales the JSON view, it could be remembered. I'm not so sure about other expansions, since when the user scales a @timestamp field, the dimension won't fit a long message field. But just loud thinking, if we could remember the last 10 fields a user scaled.

For a first version, a small improvement, I think it could make sense just to have a PR that scales the JSON content in a larger way like in this PR.

andreadelrio · 2023-04-13T21:35:18Z

Cool idea and it seems to be working well overall! Although I think it's something we should ask for design input on before we settle on a solution.

+1 from my side, @andreadelrio can you have a look? link

This looks good to me. I would just suggest, when you increase the width of the popover, keep the Filter for and Filter out buttons aligned to the left.

Also, should we have a minimum height? A height that's so low that hides the button is not ideal imo.

jughosta · 2023-04-18T16:30:15Z

Moving back to Draft state to work on improvements.

…pover

kibana-ci · 2023-05-03T17:00:03Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: 17cbe9b

Failed CI Steps

Test Failures

[job] [logs] FTR Configs #2 / dashboard panel titles by reference unlinking a by reference panel with a custom title will keep the current title
[job] [logs] FTR Configs #3 / saved objects spaces only enabled _resolve_import_errors within the default space "before all" hook for "should return 200 success [isolatedtype/defaultspace-isolatedtype-id,sharedtype/all_spaces,sharedtype/default_and_space_1,sharedtype/only_space_1,sharedtype/only_space_2,sharecapabletype/only_default_space,sharecapabletype/only_space_1,globaltype/globaltype-id,hiddentype/any,sharedtype/conflict_1a,sharedtype/conflict_1b,sharedtype/conflict_2c,sharedtype/conflict_2d,sharedtype/conflict_3a,sharedtype/conflict_4,sharedtype/outbound-missing-reference-conflict-1,sharedtype/outbound-missing-reference-conflict-2,sharedtype/outbound-reference-origin-match-1,sharedtype/outbound-reference-origin-match-2]"

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`discover`	460	461	+1

Async chunks

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

id	before	after	diff
`discover`	426.1KB	427.6KB	+1.4KB

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`enterpriseSearch`	19	21	+2
`securitySolution`	398	401	+3
total			+5

Total ESLint disabled count

id	before	after	diff
`enterpriseSearch`	20	22	+2
`securitySolution`	478	481	+3
total			+5

History

💚 Build #119300 succeeded 6776534
💚 Build #118049 succeeded ff5832c
💔 Build #111435 failed 1447383

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

cc @jughosta

jughosta added 3 commits March 1, 2023 16:30

[Discover] Resizable cell popover and larger JSON popover

9cdf026

[Discover] Some updates

2dea2a3

[Discover] Some updates

1447383

jughosta added release_note:enhancement backport:skip This commit does not require backporting Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. labels Mar 1, 2023

jughosta self-assigned this Mar 1, 2023

cee-chen mentioned this pull request Mar 6, 2023

[EuiDataGrid] Add new setCellPopoverProps API to renderCellPopover elastic/eui#6632

Merged

6 tasks

jughosta added 6 commits April 5, 2023 15:29

Merge remote-tracking branch 'upstream/main' into 132309-grid-cell-po…

cc769f3

…pover

[Discover] Use eui api

652b845

[Discover] Update styles

cc69478

[Discover] Remove redundant class

5555c83

[Discover] Update snapshots

4afb920

[Discover] Add tests

ff5832c

jughosta changed the title ~~[Discover] Optimize size of popover of Document Explorer cell expansion PoC~~ [Discover] Optimize popover size for grid cell expansion Apr 5, 2023

jughosta marked this pull request as ready for review April 5, 2023 16:51

jughosta requested review from a team as code owners April 5, 2023 16:51

davismcphee reviewed Apr 6, 2023

View reviewed changes

Merge branch 'main' into 132309-grid-cell-popover

6776534

kertal reviewed Apr 12, 2023

View reviewed changes

jughosta marked this pull request as draft April 18, 2023 16:30

jughosta added 2 commits May 3, 2023 15:01

Merge remote-tracking branch 'upstream/main' into 132309-grid-cell-po…

b9d945d

…pover

[Discover] Change the signature

17cbe9b

kertal mentioned this pull request Jul 28, 2023

Increase size of window when interacting with cell content #162320

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Discover] Optimize popover size for grid cell expansion #152480

[Discover] Optimize popover size for grid cell expansion #152480

jughosta commented Mar 1, 2023 •

edited

Loading

jughosta commented Mar 1, 2023 •

edited

Loading

kertal commented Mar 2, 2023

jughosta commented Mar 2, 2023

cee-chen commented Mar 2, 2023

jughosta commented Mar 6, 2023

cee-chen commented Mar 6, 2023

elasticmachine commented Apr 5, 2023

davismcphee left a comment

davismcphee Apr 6, 2023

kertal Apr 12, 2023

kertal commented Apr 12, 2023

andreadelrio commented Apr 13, 2023 •

edited

Loading

jughosta commented Apr 18, 2023 •

edited

Loading

kibana-ci commented May 3, 2023

ESLint disabled line counts

Total ESLint disabled count

[Discover] Optimize popover size for grid cell expansion #152480

Are you sure you want to change the base?

[Discover] Optimize popover size for grid cell expansion #152480

Conversation

jughosta commented Mar 1, 2023 • edited Loading

Summary

jughosta commented Mar 1, 2023 • edited Loading

kertal commented Mar 2, 2023

jughosta commented Mar 2, 2023

cee-chen commented Mar 2, 2023

jughosta commented Mar 6, 2023

cee-chen commented Mar 6, 2023

elasticmachine commented Apr 5, 2023

davismcphee left a comment

Choose a reason for hiding this comment

davismcphee Apr 6, 2023

Choose a reason for hiding this comment

kertal Apr 12, 2023

Choose a reason for hiding this comment

kertal commented Apr 12, 2023

andreadelrio commented Apr 13, 2023 • edited Loading

jughosta commented Apr 18, 2023 • edited Loading

kibana-ci commented May 3, 2023

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Module Count

Async chunks

ESLint disabled line counts

Total ESLint disabled count

History

jughosta commented Mar 1, 2023 •

edited

Loading

jughosta commented Mar 1, 2023 •

edited

Loading

andreadelrio commented Apr 13, 2023 •

edited

Loading

jughosta commented Apr 18, 2023 •

edited

Loading