[EuiDataGrid] Add rowHeightSwitcher logic + API change #5372

cee-chen · 2021-11-10T23:03:07Z

Summary

This is a fairly hefty PR and I strongly recommend following along by commit.

TL;DR:

Deprecated showStyleSelector in favor of showDisplaySelector
New showDisplaySelector nested config options: { allowDensity: true, allowRowHeight: true }
Fixed undefined/"Single" row heights to correctly account for density by changing them to also use this.props.setRowHeight to dynamically adjust their minRowHeight (basically re-using the same logic as lineCount row heights)

Screencaps

Code coverage

Checklist

Check against all themes for compatibility in both light and dark modes
Checked in mobile
Checked in Chrome, Safari, Edge, and Firefox
Props have proper autodocs and playground toggles
Added documentation

~~- [ ] Checked Code Sandbox works for any docs examples~~

Added or updated jest and cypress tests

NOTE: Cypress testing will likely need #5371 for styles - leaving this as a tech debt/backlog item

Checked for breaking changes and labeled appropriately
Checked for accessibility including keyboard-only and screenreader modes
A changelog entry exists and is marked appropriately

kibanamachine · 2021-11-10T23:33:39Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5372/

kibanamachine · 2021-11-11T18:36:19Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5372/

- so that both the columnSelector and displaySelector can use it

+ update docs: - reorder UI to match toolbar layout - fix legends + add unit tests (& improve some existing unit tests)

NYI: conditional lineCount controls + unit tests

+ fix cell height to dynamically on lineCount change

…efined heights but defined rowHeightsOptions objects e.g. - empty objs, or lineHeight set but nothing else + simplify this.props

…ined mode - Multiline content includes items with ` `/`` tags etc that end up incorrectly vertically centered to their increased height

kibanamachine · 2021-11-11T20:21:00Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5372/

… + density - by giving it the same setRowHeight logic as line counts with a single line + rename recalculateLineCountHeight for clarity now that it's no longer being used for just lineCount

kibanamachine · 2021-11-11T23:06:27Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5372/

cee-chen · 2021-11-11T23:46:34Z

src/components/datagrid/_data_grid_data_row.scss

@@ -139,7 +140,7 @@
 .euiDataGridRowCell__expandFlex {
  position: relative; // for positioning expand button
  display: flex;
-  align-items: center;
+  align-items: baseline;


Without this fix (be59b08), cell contents that are multiline due to  s/s/other non-text-wrapping means would not correctly be aligned on 'Single'/undefined row heights:

Before (center-aligned)

After (top/base-aligned)

Note that this actually already an issue on production, this PR simply makes it more obvious because the row height switcher easily allows users to switch to single/undefined row heights on multi-line content, whereas previously a consumer would likely set a static row height or lineCount.

Also ignore the incorrect line height in this screenshot, this is an issue that gets fixed in the next commit.

cee-chen · 2021-11-12T00:02:08Z

src/components/datagrid/body/data_grid_cell.tsx

+    const isSingleLine = rowHeightOption == null; // Undefined rowHeightsOptions default to a single line
+    const lineCount = isSingleLine
+      ? 1
+      : rowHeightUtils?.getLineCount(rowHeightOption);


This fix/workaround essentially has the single/undefined row heights act as lineCount of 1, which allows single rows to take advantage of lineCount's ability to get cell heights/font sizes/line heights and dynamically respond to density changes.

Before:

The non-dynamic height changing is even more noticeable now that single row heights are no longer center aligned (per previous commit/fix) and now that you can compare lineCount directly against undefined, which does correctly reset the minimum row height based on density.

After:

I initially played with an approach that used a static minRowHeight map depending on density and exporting that from the display selector - but I think this approach is much simpler/better because it's using the logic we already have, and also accounts for row/line height differences between the legacy theme and Amsterdam theme.

This also fixes an issue that occurs when switching from lineCount to undefined heights - the undefined heights' minRowHeight was not being correctly reset, so this approach basically kills two birds with one stone.

- to make the difference more obvious when switching between single and custom

kibanamachine · 2021-11-16T18:35:41Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5372/

cee-chen · 2021-11-16T19:19:41Z

jenkins test this

kibanamachine · 2021-11-16T19:46:58Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5372/

…to datagrid-row-height-control

kibanamachine · 2021-11-17T02:49:21Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5372/

cchaos · 2021-11-17T17:16:22Z

So I'm noticing a bit of a discrepancy between how the "Single" and "Auto fit" behave when there is only a single line of content. I'd expect the heights of the cells to continue to be the same. Also in compressed style, I think the height is too high when "Auto fit".

Screen.Recording.2021-11-17.at.12.13.18.PM.mp4

EDIT: It looks like this stems from the "Single" version version not taking the actions column into account so the button is getting cutoff.

cee-chen · 2021-11-17T20:05:09Z

Just to clarify, "Single" heights have never taken the actions column into account, what they did was just vertically center all of their content which was fine for things like buttons or text that never had manual   type breaks in them. The problem was also not as noticeable on compressed densities because we previously enforced a minimum height of 34px on single heights.

I think the solution here is to tweak control columns buttons by reducing their size or padding/margins on compressed density (so it's not causing the height to be too tall on auto, and so it's correctly aligned on single/lineCount mode).

EDIT: Actually on second thought https://eui.elastic.co/pr_5372/#/tabular-content/data-grid-control-columns/ is fairly derpy as well (primarily checkboxes) even on normal density so I think the perhaps more robust solution is to switch control columns only back to a center vertical alignment (since theoretically those columns should not contain multi-line text).

EDIT 2: Actually both is required. Fun times!

chandlerprall · 2021-11-23T21:28:01Z

Then when the user selects an option, change it for all the rows even the very specifically set by index ones.

I'm hesitant to agree with this premise. I somewhat deliberately left the rowHeights overrides untouched by the new row height UI, which only sets the defaultHeight of the grid. I didn't see this as functionality that was requested or needed by the Discover team in #5080 (although we can certainly ask!). I think my hesitation here is that rowHeights is clearly billed as an override, and it feels strange to have an override that's so easily overrideable by the user. It's certainly technically doable, I just want to make sure it's a deliberate choice we're making (and also that we document it super clearly).

This is tricky, and I flopped back and forth a bit in that section during my review. Looking ahead to the ask for row renderers, I can see a world where there are "rows" and ... "extra rows" (conceptually in the application, not within EUI code), with the extra rows getting an auto height while the rows use the default set by either application or user. Based on this, I lean toward the existing implementation where the override affects only the default. In theory, there's a really good reason for the application to set a specific height on something.

I also appreciate the scenario where a user is confused when only a subset of rows are affected by their choice.

cee-chen · 2021-11-23T21:42:14Z

If a consumer has deliberately chose specific heights for specific rows, my guess is they don't want the consumer to adjust the row heights at all.

Those are good points! Could we configure the specific rows example to turn off showDisplaySelector.allowRowHeight, and add a documentation note that we recommend disabling that configuration if setting individual row height overrides?

Is that a slightly happier medium between programmatically enforcing/disabling the row height switcher whenever a rowHeights object override is set?

…to datagrid-row-height-control

cchaos · 2021-11-23T21:55:04Z

So something like?:

rowHeightsOptions = {
  defaultHeight: 140, // Row heights selector still shows but unselected
  rowHeights: {
    1: {
      lineCount: 5,
      allowRowHeight: false, // Ensures it doesn't change based on row height selector
    },
    4: 200, // Will adjust based on row height selector, this is just the starting height
  },
}

kibanamachine · 2021-11-23T22:13:18Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5372/

cee-chen · 2021-11-23T22:48:30Z

Not quite sorry, I just meant changing the actual src-docs/../row_height_fixed.tsx example itself to disable toolbar config:

<EuiDataGrid
  // ...
  toolbarVisibility={{
    showDisplaySelector: { allowRowHeight: false },
  }}
  rowHeightsOptions={rowHeightsOptions} // has rowHeights overrides
/>

cchaos · 2021-11-23T23:05:42Z

Ok, as a compromise, add that to the docs example with a note in the description (and the props comment for rowHeightsOptions) that we recommend also turning off allowRowHeight. Then let's make an issue to specifically address this as a follow up. I think I still like my suggestion where we enforce a good UX but consumers can opt-out. The issue doesn't have to be addressed in this feature branch either.

cchaos

I'll apply my pre-approval now in case the rest of them come in while I'm out. 😅 Awesome job!! I think the final UI is intuitive and I'm glad we, but mostly you, took the time to work through all the rest of the toolbar stuff to go with it.

+ improve unit tests to cover undefined nested object key case

kibanamachine · 2021-11-24T16:42:12Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5372/

@see

…er when using `rowHeights` @see elastic#5411

kibanamachine · 2021-11-24T17:16:44Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5372/

cee-chen · 2021-11-24T17:27:06Z

rowHeights documentation and issue:

chandlerprall

LGTM! Read through & tested the updated examples, looks like all review feedback has been addressed.

kibanamachine · 2021-11-24T17:53:16Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5372/

chandlerprall · 2021-11-24T17:58:35Z

Interesting Cypress failure, gonna see if a re-run passes; jenkins test this

10:43:44 EuiDataGrid
10:43:44 row creation
10:43:44 (Attempt 1 of 3) creates rows
10:43:45 1) "after all" hook: mergeUnitTestCoverage for "creates rows"
10:44:09
10:44:09
10:44:09 0 passing (25s)
10:44:09 1 failing
10:44:09
10:44:09 1) EuiDataGrid
10:44:09 row creation
10:44:09 "after all" hook: mergeUnitTestCoverage for "creates rows":
10:44:09 Error: The following error originated from your test code, not from Cypress.
10:44:09
10:44:09 > ResizeObserver loop limit exceeded

kibanamachine · 2021-11-24T18:24:28Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5372/

cee-chen · 2021-11-24T19:03:10Z

It's failed a couple times now on this branch since latest main was merged in, so unfortunately I think it might be a persistent failure. I can repro it locally as well. An easy fix would just be to ignore the specific ResizeObserver loop limit error, but I'm a little worried it's somehow related to changes in this branch (although I can't really see how it would specifically affect E2E tests)

cee-chen · 2021-11-24T19:42:10Z

HA, I've apparently run into this exact same ResizeObserver loop error in Enterprise Search:

https://github.com/elastic/kibana/blob/8dfdeadd4054bc53e989b34d8a1c06f752a36e47/x-pack/plugins/enterprise_search/public/applications/enterprise_search/cypress/integration/overview.spec.ts#L34-L38

The security team also has a workaround for it:

https://github.com/elastic/kibana/blob/a93a7efa8061f840ffc28394bdec86f51937314e/x-pack/plugins/apm/ftr_e2e/cypress/support/index.ts#L8-L14

With that in mind + the responses from https://stackoverflow.com/questions/49384120/resizeobserver-loop-limit-exceeded I feel relatively safe pushing up a general return false for this error.

Also see https://stackoverflow.com/questions/49384120/resizeobserver-loop-limit-exceeded

kibanamachine · 2021-11-24T20:11:37Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5372/

chandlerprall

🙌 thanks for tracking down all that info

…5415) * [EuiDataGrid] Toolbar UI layout reorganization (#5334) * [EuiDataGrid] Add extra left prepend position to the `additionalControls` API (#5394) * [EuiDataGrid] Add rowHeightSwitcher logic + API change (#5372) * [EuiDataGrid] Add `onChange` callbacks for display selector changes (#5424) * [EuiDataGrid] Row height switcher should override `rowHeights` + add reset button (#5428) * [EuiDataGrid] improve height calculation (#5447) Co-authored-by: Caroline Horn <[email protected]> Co-authored-by: Chandler Prall <[email protected]>

cee-chen force-pushed the datagrid-row-height-control branch from 89b2783 to 2057eaf Compare November 11, 2021 17:59

cee-chen force-pushed the datagrid-row-height-control branch from 0746aae to 419ed6b Compare November 11, 2021 19:25

cee-chen and others added 7 commits November 11, 2021 11:54

Rename styleSelector to displaySelector + update types & docs

916c1c7

Refactor out nested object helper

79c878d

- so that both the columnSelector and displaySelector can use it

Update displaySelector with conditional density/row height options

58fe94f

+ update docs: - reorder UI to match toolbar layout - fix legends + add unit tests (& improve some existing unit tests)

Add rowHeightsOptions controls to popover

37aecd6

NYI: conditional lineCount controls + unit tests

Add conditional lineCount row and logic

129b2bb

+ fix cell height to dynamically on lineCount change

Fix styles not applying correctly for rowHeightsOptions that have und…

742051d

…efined heights but defined rowHeightsOptions objects e.g. - empty objs, or lineHeight set but nothing else + simplify this.props

Fix multiline content not top-aligning correctly when in single/undef…

be59b08

…ined mode - Multiline content includes items with ` `/`` tags etc that end up incorrectly vertically centered to their increased height

cee-chen force-pushed the datagrid-row-height-control branch from 419ed6b to fd18b72 Compare November 11, 2021 19:54

cee-chen added 2 commits November 11, 2021 14:35

Fix single/undefined row heights to account for passed lineHeight API…

75e933a

… + density - by giving it the same setRowHeight logic as line counts with a single line + rename recalculateLineCountHeight for clarity now that it's no longer being used for just lineCount

Add changelog entry

39aa176

cee-chen force-pushed the datagrid-row-height-control branch from fd18b72 to 39aa176 Compare November 11, 2021 22:35

cee-chen added the data grid label Nov 11, 2021

cee-chen marked this pull request as ready for review November 11, 2021 23:24

cee-chen commented Nov 11, 2021

View reviewed changes

cee-chen commented Nov 12, 2021

View reviewed changes

[PR feedback] Increase default lineCount to 2

8580dc8

- to make the difference more obvious when switching between single and custom

cee-chen added 2 commits November 16, 2021 18:15

Merge branch 'feat/datagrid/toolbar-reorg-and-row-height-switcher' in…

0a4eae8

…to datagrid-row-height-control

Fix changelog

7e34537

Merge branch 'feat/datagrid/toolbar-reorg-and-row-height-switcher' in…

be0db10

…to datagrid-row-height-control

cchaos approved these changes Nov 23, 2021

View reviewed changes

[PR feedback] Improve typing to avoid any usage

fc9f65c

+ improve unit tests to cover undefined nested object key case

cee-chen mentioned this pull request Nov 24, 2021

[EuiDataGrid] How should the row height switcher toolbar control handle rowHeights overrides? #5411

Closed

[PR feedback] Document recommendation for disabling row height switch…

4250a73

…er when using `rowHeights` @see elastic#5411

[Pr feedback] Simplify getNestedObjectOptions

fc4105c

cee-chen requested a review from chandlerprall November 24, 2021 17:29

chandlerprall approved these changes Nov 24, 2021

View reviewed changes

Add workaround for failing Cypress tests

0b9ca21

Also see https://stackoverflow.com/questions/49384120/resizeobserver-loop-limit-exceeded

chandlerprall approved these changes Nov 24, 2021

View reviewed changes

cee-chen merged commit ff742c6 into elastic:feat/datagrid/toolbar-reorg-and-row-height-switcher Nov 24, 2021

cee-chen deleted the datagrid-row-height-control branch November 24, 2021 21:01

cee-chen mentioned this pull request Nov 24, 2021

[EuiDataGrid] Feature: toolbar reorganization and row height control #5415

Merged

10 tasks

thompsongl mentioned this pull request Dec 9, 2021

Upgrade EUI to v43.1.1 elastic/kibana#120935

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EuiDataGrid] Add rowHeightSwitcher logic + API change #5372

[EuiDataGrid] Add rowHeightSwitcher logic + API change #5372

cee-chen commented Nov 10, 2021 •

edited

Loading

kibanamachine commented Nov 10, 2021

kibanamachine commented Nov 11, 2021

kibanamachine commented Nov 11, 2021

kibanamachine commented Nov 11, 2021

cee-chen Nov 11, 2021

cee-chen Nov 12, 2021 •

edited

Loading

kibanamachine commented Nov 16, 2021

cee-chen commented Nov 16, 2021

kibanamachine commented Nov 16, 2021

kibanamachine commented Nov 17, 2021

cchaos commented Nov 17, 2021 •

edited

Loading

cee-chen commented Nov 17, 2021 •

edited

Loading

chandlerprall commented Nov 23, 2021

cee-chen commented Nov 23, 2021 •

edited

Loading

cchaos commented Nov 23, 2021

kibanamachine commented Nov 23, 2021

cee-chen commented Nov 23, 2021

cchaos commented Nov 23, 2021

cchaos left a comment

kibanamachine commented Nov 24, 2021

kibanamachine commented Nov 24, 2021

cee-chen commented Nov 24, 2021

chandlerprall left a comment

kibanamachine commented Nov 24, 2021

chandlerprall commented Nov 24, 2021

kibanamachine commented Nov 24, 2021

cee-chen commented Nov 24, 2021

cee-chen commented Nov 24, 2021

kibanamachine commented Nov 24, 2021

chandlerprall left a comment

[EuiDataGrid] Add rowHeightSwitcher logic + API change #5372

[EuiDataGrid] Add rowHeightSwitcher logic + API change #5372

Conversation

cee-chen commented Nov 10, 2021 • edited Loading

Summary

Screencaps

Code coverage

Checklist

kibanamachine commented Nov 10, 2021

kibanamachine commented Nov 11, 2021

kibanamachine commented Nov 11, 2021

kibanamachine commented Nov 11, 2021

cee-chen Nov 11, 2021

Choose a reason for hiding this comment

Before (center-aligned)

After (top/base-aligned)

cee-chen Nov 12, 2021 • edited Loading

Choose a reason for hiding this comment

Before:

After:

kibanamachine commented Nov 16, 2021

cee-chen commented Nov 16, 2021

kibanamachine commented Nov 16, 2021

kibanamachine commented Nov 17, 2021

cchaos commented Nov 17, 2021 • edited Loading

cee-chen commented Nov 17, 2021 • edited Loading

chandlerprall commented Nov 23, 2021

cee-chen commented Nov 23, 2021 • edited Loading

cchaos commented Nov 23, 2021

kibanamachine commented Nov 23, 2021

cee-chen commented Nov 23, 2021

cchaos commented Nov 23, 2021

cchaos left a comment

Choose a reason for hiding this comment

kibanamachine commented Nov 24, 2021

kibanamachine commented Nov 24, 2021

cee-chen commented Nov 24, 2021

chandlerprall left a comment

Choose a reason for hiding this comment

kibanamachine commented Nov 24, 2021

chandlerprall commented Nov 24, 2021

kibanamachine commented Nov 24, 2021

cee-chen commented Nov 24, 2021

cee-chen commented Nov 24, 2021

kibanamachine commented Nov 24, 2021

chandlerprall left a comment

Choose a reason for hiding this comment

cee-chen commented Nov 10, 2021 •

edited

Loading

cee-chen Nov 12, 2021 •

edited

Loading

cchaos commented Nov 17, 2021 •

edited

Loading

cee-chen commented Nov 17, 2021 •

edited

Loading

cee-chen commented Nov 23, 2021 •

edited

Loading