[Discover] Add row height options by dimaanj · Pull Request #122087 · elastic/kibana

dimaanj · 2021-12-28T16:32:46Z

Summary

This PR adds ability to change row height options of Document explorer.

⚠️ Current PR still contains one small issue related to Eui component. Once user change rowHeight using toolbar, updating rowHeight from URL will not work.

Priority for applying rowHeight

URL state
Saved search
Last used value (localStorage) or advanced setting (uiSettings)

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

dimaanj · 2022-01-03T19:47:54Z

@elasticmachine merge upstream

elasticmachine · 2022-01-04T07:40:24Z

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

kertal · 2022-01-05T07:29:15Z

@elasticmachine merge upstream

kertal

Started testing and I'm excited that we will soon support this long awaited functionality in Discover. Sharing my thoughts while testing:

I think default Density should be normal, and we should add this value to local storage, so users don't have to reconfigure it every time. for me it looks a bit buggy when switching e.g. to Expanded in Safari (Chrome seems to be fine):

Via the URL I was able to configure invalid states like -1 , also 10000, so we need better validation here
There's a difference in the way what's being displayed when selecting Single or Custom set to 1

In the current way this is implemented we can't store single, since a given row height of 1 is automatically assumed to be a custom height of 1, asking @elastic/kibana-design for feedback if we should also support single, in this case we would need to rethink the way we handle persistence in state and saved object.

if we would need to support single we could do it the following way mapping to integers
-1 : auto height
0: single
1: custom 1
2: custom 2
.....

src/plugins/discover/server/ui_settings.ts

src/plugins/discover/public/embeddable/saved_search_embeddable_component.tsx

andreadelrio · 2022-01-05T19:12:06Z

In the current way this is implemented we can't store single, since a given row height of 1 is automatically assumed to be a custom height of 1, asking @elastic/kibana-design for feedback if we should also support single, in this case we would need to rethink the way we handle persistence in state and saved object.

I think given that 1 is the default value for Custom, Single might not be needed. If we do want to support Single (as it might be a frequently needed setting) I would suggest we make 2 the default value for Custom as seen in the DataGrid's EUI docs examples.

andreadelrio · 2022-01-05T20:40:40Z

I think default Density should be normal, and we should add this value to local storage, so users don't have to reconfigure it every time.

+1. I also think Normal should be the default, seems readability is better and perhaps matches Legacy more closely.

We currently have an issue with the font sizes when we display the whole document (i.e. no columns have been added). The EuiDataGrid styles are overridden by EuiDescriptionList's styles and I think we should address this. See:

Also when changing the Density there's an issue with the padding next to the checkbox to select documents.

For the expand/minimize icons it would be nice if for Expanded density we could do EuiButtonIcon with iconSize "m".

kertal · 2022-01-11T08:24:22Z

I think default Density should be normal, and we should add this value to local storage, so users don't have to reconfigure it every time.

+1. I also think Normal should be the default, seems readability is better and perhaps matches Legacy more closely.

We currently have an issue with the font sizes when we display the whole document (i.e. no columns have been added). The EuiDataGrid styles are overridden by EuiDescriptionList's styles and I think we should address this. See:

@andreadelrio Yes, we should fix the mixtures of font-sizes. But I think we should stick to a single size, and that's the size that's being displayed when choosing compact. When I look at the expanded view, especially with the Document column the font size is very large compared to the rest of the page.

Personally, if I wanted a larger font size I'd rather use the browsers zoom functionality, to enlarge the whole page. So for me density shouldn't modify font size, but just padding. This matches better to the classic way.

@dmitriynj
Like Adrea mentioned the expand and selection columns should be fixed when using Normal & Expanded

kertal · 2022-01-11T08:27:06Z

src/plugins/discover/server/ui_settings.ts

+    name: i18n.translate('discover.advancedSettings.params.rowHeightTitle', {
+      defaultMessage: 'Maximum lines of document explorer cell',
+    }),


I'd suggest switching to
Displayed lines of document explorer cell
FYI @gchaps

Like @gchaps suggested this should be changed into Row height in the Document Explorer

kertal · 2022-01-11T08:30:53Z

src/plugins/discover/server/ui_settings.ts

+    category: ['discover'],
+    description: i18n.translate('discover.advancedSettings.params.rowHeightText', {
+      defaultMessage:
+        'The maximum lines that a cell in a table should occupy. Auto height might be enabled by setting value to -1. Single option could be enabled by setting value to 0.',


I'd suggest to change to
Displayed lines of a document explorer's cell. Auto height can be enabled by setting the value to -1. The explorer's single option can be enabled by setting value to 0.
FYI @gchaps

@kertal In looking at other documents, the term "row height" seems to be appropriate here. It's also the word you use in the pop-up. Here is a suggestion:

Row height in the Document Explorer
The number of lines to allow in a row. A value of -1 automatically adjusts the row height to fit the contents. A value of 0 displays the content in a single line.

I'm a little confused over the term Single and whether it is the same as setting Custom to 1.

@gchaps sounds good, yes "single". Thing is you can configure the height in the explorer to show just 1 line in 2 ways. Either you could choose Single or Custom with a height of 1. They look a bit different in terms of truncation, and work differently under the hood

andreadelrio · 2022-01-19T00:33:23Z

@andreadelrio sounds good, question is if we should wait for elastic/eui#5526 to land in Kibana, which means, merge, once both is available, let's discuss it in our weekly

I think we're ok to wait for both to be available. I'll make sure to follow up to have mine approved asap.

…-height-options # Conflicts: # src/plugins/discover/public/application/context/context_app.test.tsx # src/plugins/discover/public/application/main/components/layout/discover_documents.test.tsx # src/plugins/discover/public/application/main/components/layout/discover_layout.test.tsx # src/plugins/discover/public/components/discover_grid/discover_grid.test.tsx # src/plugins/discover/public/components/discover_grid/discover_grid.tsx # src/plugins/discover/public/embeddable/saved_search_grid.tsx

kertal · 2022-01-31T09:38:24Z

@elasticmachine merge upstream

…searches

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

andreadelrio · 2022-01-31T19:22:31Z

@dmitriynj thanks for the updates, it is looking better. Wanted to check why content seems to be getting cut off when setting a custom # of lines? See below for the 3 lines case:

kertal · 2022-02-01T08:16:08Z

@dmitriynj thanks for the updates, it is looking better. Wanted to check why content seems to be getting cut off when setting a custom # of lines? See below for the 3 lines case:

I think I know why, when you disable the following in the browser it works:

But I don't know how to disable it permanently?

kertal

Code LGTM, great work! Tested remote using Firefox, Safari, Edge, Chrome. Works as expected with the following limitations:

As mentioned in the description, once row height is changed locally, this state can't be overwritten by URL state, this should be fixed on EUI level
As discussed when showing the Document column with a certain number of lines, the last line gets cut off, this will high likely be a CSS fix somewhere? @andreadelrio
The reset to default makes no sense for our case, but that's also an EUI issue https://user-images.githubusercontent.com/463851/152003576-34d283c2-c413-44d8-8353-9ff2c554e244.mp4
And since we now display the loading state when loading a saved search, I encountered that when loading e.g. an e-commerce saved search followed by a flights saved search, when navigating back using the browsers back button, the loading state is displayed twice and also data is requested twice ... one load is triggered because the index pattern of the url changes, the other because of the change of the saved search. It shows a weakness of our current state handling, shouldn't be a blocker, but we should improve it.

All in all, don't think any of this should block us from merging, but of course we should fix those issues. We want flags, cats, dogs with flexible height in the Document explorer!

andreadelrio · 2022-02-01T19:31:22Z

I was looking at a CSS override for the line-height but I think a cleaner way to solve this is using EuiDataGrid settings, so I've set lineHeight in EuiDataGridRowHeightsOptions. See the result below:

Full doc, 3 lines per row:

Table with columns:

@kertal @dmitriynj let me know what you think and if there are any risks with this approach.

ryankeairns

Since Andrea contributed, and I've verified locally, going ahead with an approval from design. This looks really nice, great work!

kertal · 2022-02-02T04:18:12Z

@elasticmachine merge upstream

kibana-ci · 2022-02-02T05:47:02Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`discover`	368	371	+3

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`discover`	62	65	+3

Async chunks

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

id	before	after	diff
`discover`	334.0KB	335.4KB	+1.4KB

Page load bundle

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

id	before	after	diff
`discover`	51.7KB	51.8KB	+123.0B

Saved Objects .kibana field count

Every field in each saved object type adds overhead to Elasticsearch. Kibana needs to keep the total field count below Elasticsearch's default limit of 1000 fields. Only specify field mappings for the fields you wish to search on or query. See https://www.elastic.co/guide/en/kibana/master/development-plugin-saved-objects.html#_mappings

id	before	after	diff
`search`	12	13	+1

Unknown metric groups

API count

id	before	after	diff
`discover`	90	93	+3

History

💔 Build #21155 failed c301fa3
💚 Build #20965 succeeded c876470
💚 Build #20859 succeeded 92b8340
💚 Build #20803 succeeded bf6454a
💔 Build #20762 failed ef7b1d8

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

cc @dmitriynj

kertal · 2022-02-02T09:33:46Z

@andreadelrio it's a nice solution, and no hack 👍 thx so much!

[Discover] add row height options

1aa022f

dimaanj added Feature:Discover Discover Application release_note:feature Makes this part of the condensed release notes Team:DataDiscovery Discover, search (data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. t// v8.2.0 labels Dec 28, 2021

dimaanj self-assigned this Dec 28, 2021

dimaanj added 3 commits January 3, 2022 14:40

[Discover] fix build problems, add unit tests

c73d3f1

[Discover] update unit test snapshots

40cdf50

[Discover] fix unit test and functional test

17b027b

Merge branch 'main' into add-row-height-options

51e18ad

dimaanj marked this pull request as ready for review January 4, 2022 07:40

dimaanj requested review from a team as code owners January 4, 2022 07:40

kertal requested a review from a team January 5, 2022 06:20

kertal added v8.1.0 and removed v8.2.0 labels Jan 5, 2022

Merge branch 'main' into add-row-height-options

db09574

kertal reviewed Jan 5, 2022

View reviewed changes

src/plugins/discover/server/ui_settings.ts Outdated Show resolved Hide resolved

kertal reviewed Jan 5, 2022

View reviewed changes

src/plugins/discover/public/embeddable/saved_search_embeddable_component.tsx Outdated Show resolved Hide resolved

dimaanj added 2 commits January 10, 2022 20:08

[Discover] fix density, rowHeight when switching savedSearch

51de741

[Discover] remove redundant prop

717ce9d

kertal reviewed Jan 11, 2022

View reviewed changes

dimaanj added 2 commits January 19, 2022 12:19

[Discover] temporary fix

0636d4f

[Discover] fix local row height init

b3b0737

Heenawter mentioned this pull request Jan 28, 2022

[Spacetime] [Discover] Add demo of possible in-product help to Discover #123888

Closed

kibanamachine and others added 7 commits January 31, 2022 04:38

Merge branch 'main' into add-row-height-options

75a7960

[Discover] return density, remove redundant leftovers

fc13850

[Discover] fix unit test

ef7b1d8

[Discover] update unit tests

bf6454a

[Discover] introduce loading view for data grid when switching saved …

62a2dc6

…searches

[Discover] remove density

c0d291c

[Discover] update advanced settings wording

92b8340

kertal reviewed Jan 31, 2022

View reviewed changes

src/plugins/discover/public/components/discover_grid/discover_grid.tsx Show resolved Hide resolved

cqliu1 approved these changes Jan 31, 2022

View reviewed changes

[Discover] apply suggestion

c876470

kertal approved these changes Feb 1, 2022

View reviewed changes

fix lineHeight

c301fa3

ryankeairns approved these changes Feb 1, 2022

View reviewed changes

Merge branch 'main' into add-row-height-options

2c470ce

kertal merged commit a7e0171 into elastic:main Feb 2, 2022

kibanamachine added the backport:skip This PR does not require backporting label Feb 2, 2022

kertal mentioned this pull request Feb 3, 2022

[EuiDataGrid] Reset internal state of row height / density popover when props change elastic/eui#5593

Closed

jughosta mentioned this pull request Oct 28, 2022

Configurable page size, number of pages, and field trim for doc table #4060

Closed

Conversation

dimaanj commented Dec 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Priority for applying rowHeight

Checklist

Uh oh!

dimaanj commented Jan 3, 2022

Uh oh!

elasticmachine commented Jan 4, 2022

Uh oh!

kertal commented Jan 5, 2022

Uh oh!

kertal left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

andreadelrio commented Jan 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andreadelrio commented Jan 5, 2022

Uh oh!

kertal commented Jan 11, 2022

Uh oh!

kertal Jan 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kertal Jan 14, 2022

Choose a reason for hiding this comment

Uh oh!

kertal Jan 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gchaps Jan 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kertal Jan 12, 2022

Choose a reason for hiding this comment

Uh oh!

andreadelrio commented Jan 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kertal commented Jan 31, 2022

Uh oh!

Uh oh!

andreadelrio commented Jan 31, 2022

Uh oh!

kertal commented Feb 1, 2022

Uh oh!

kertal left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andreadelrio commented Feb 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryankeairns left a comment

Choose a reason for hiding this comment

Uh oh!

kertal commented Feb 2, 2022

Uh oh!

kibana-ci commented Feb 2, 2022

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Page load bundle

Saved Objects .kibana field count

API count

History

Uh oh!

kertal commented Feb 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dimaanj commented Dec 28, 2021 •

edited

Loading

kertal left a comment •

edited

Loading

andreadelrio commented Jan 5, 2022 •

edited

Loading

kertal Jan 11, 2022 •

edited

Loading

kertal Jan 11, 2022 •

edited

Loading

gchaps Jan 11, 2022 •

edited

Loading

andreadelrio commented Jan 19, 2022 •

edited

Loading

kertal left a comment •

edited

Loading

andreadelrio commented Feb 1, 2022 •

edited

Loading

kertal commented Feb 2, 2022 •

edited

Loading