[EuiDataGrid] Toolbar UI layout reorganization #5334

cee-chen · 2021-10-27T22:14:34Z

Summary

This PR starts the process for updating our data grid toolbar layout to the mock in @cchaos's comment here: #5080 (comment)

I recommend following along by commit!

⚠️ The row height functionality itself is still WIP and requires 2 major bugfixes (height updating + broken computed styles in FF/Safari) before it can be completed, so this PR is just reordering/reorganizing the toolbar layout for now.

Before

After

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

~~- [ ] Checked for breaking changes and labeled appropriately~~

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

- naming was incorrect in the case of the style selector popover

- Columns & Sort on left, with additionalControls to the right of those (+ update prop types to indicate that) - Style (TBD naming) and Fullscreen to the right

+ tweak grid styles icon to match Caroline's mock + popover location

…ocks - refactor densityOptions to const outside hook

cee-chen · 2021-10-27T22:16:57Z

NB: I'm 50/50 on this, but I opted not to include any potential breaking API changes/renames (see #5080 (comment) and the next few comments) as part of this PR, although I certainly could if folks think it would be smarter to do so now instead of later 🤔

kibanamachine · 2021-10-27T22:44:38Z

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

cchaos

Columns & Sort on left, with additionalControls to the right of those (+ update prop types to indicate that)

Oh shoot, sorry about this but, when I commented about the custom controls being on the right side of the built-in ones, I was under the impression this was already how it was. I'd worry about this change for consumers without a way to customize the placement. additionalControls is already used a bunch in Kibana.

Not related to this PR, but would be a nice fix:

I noticed on the docs page:https://eui.elastic.co/pr_5334/#/tabular-content/data-grid-styling-and-control that the Show sort selector option is not doing anything. Can we fix that?

src/components/datagrid/controls/_data_grid_toolbar.scss

src/components/datagrid/controls/style_selector.tsx

src/components/datagrid/controls/_data_grid_display.scss

…iption

NB: sorting functionality won't actually work/isn't hooked up, but this should at least allow the show/hide UI toggle to work as expected

cee-chen · 2021-10-29T19:08:27Z

Oh shoot, sorry about this but, when I commented about the custom controls being on the right side of the built-in ones, I was under the impression this was already how it was. I'd worry about this change for consumers without a way to customize the placement. additionalControls is already used a bunch in Kibana.

For sure, that totally makes sense! I reverted the position change and opted to correct the props docs: 7b1158a

I noticed on the docs page:https://eui.elastic.co/pr_5334/#/tabular-content/data-grid-styling-and-control that the Show sort selector option is not doing anything. Can we fix that?

This should be fixed now! 07ea746 I just did the first example on the page that has the toggle options and not every single example on the page as a heads up

kibanamachine · 2021-10-29T19:33:19Z

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

kibanamachine · 2021-10-29T22:22:41Z

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

snide · 2021-11-01T15:41:21Z

Just a note to make sure you all communicate with the Security and Observability UI teams (@andrew-goldstein and @weltenwort would be a start). I have a feeling the new positioning will break some of their overwrites (specifically I know they abs position some items into that top right corner). This should give them better guidance for how to position those controls (by using the additional props) they need to add. Not a breaking change the way I see it delivered in EUI, but it will break in Kibana based upon some of their usage.

weltenwort · 2021-11-01T16:09:57Z

Good call, thanks for the ping. The shared alerts table has indeed customized the content. cc @elastic/observability-rac, because responsibilities are being rearranged at the moment

cee-chen · 2021-11-01T16:13:40Z

If you want your own totally custom toolbar content that's absolutely positioned to the right, you can likely still override this flex positioning as needed by targeting:

.euiDataGrid__controls {
  justify-content: flex-start !important;
}

or similar - it may look a little odd though because the right items are now icons only 🙈 @cchaos may have a better holistic solution!

cchaos · 2021-11-01T19:11:27Z

The ideal solution is just making the additionalControls more customizable by position. My suggestion is to convert this prop to either accept a ReactNode as it does now that defaults to the position of middleLeft or an object of ReactNodes with position-named keys like left, middleLeft, middleRight, right. (I'm still workshopping these names).

cee-chen · 2021-11-01T20:39:44Z

@cchaos Would you be fine with this PR proceeding without that behavior for now? I checked with Chandler this morning and technically since custom CSS with EuiDataGrid isn't officially supported, he mentioned we shouldn't let that hold up our current work - and it gives us time to more fully implement additionalControls and any custom positioning later

kibanamachine · 2021-11-01T21:11:03Z

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

cchaos · 2021-11-02T13:44:37Z

Would you be fine with this PR proceeding without that behavior for now?

Since we are already aware that this will break a specific implementation, and we know the fix, I'd rather get the fix in so it doesn't become a backlog issue.

* Update some props comments and fix popover’s responsiveness * [Design] Remove a few extraneous style overrides to control buttons And lower left padding * Fixed add/remove of fullscreen body class * Added tooltips around icon buttons with long delay * Updated examples and snippets with “real world” examples Removed all the unnecessary manual additions of `className="euiDataGrid__controlBtn”` * More responsive fixes * Change class name

- It isn't necessary for popovers without drag & drop, name becomes verbose but it's hopefully worth it for clearer usage

cee-chen · 2021-11-08T20:43:42Z

I'll let an engineer handle the addition of something like that.

Cool - that will likely be me since I appear to be primarily on datagrid 😅 Not sure if we should open #5346 back up to discuss updating the current API (e.g., leftAppend, leftPrepend) for this vs. adding a whole separate config key. controlColumnButton doesn't make a whole lot of sense to me as a toolbar control since they're not visually lined up (although maybe I'm not understanding what you're saying).

If we were to add a brand new API, maybe something like showItemsDetails and then we could use that slot to display the "X out of Y items" text & allow consumers/Discover to simply override it?

…'s work

…to datagrid-toolbar

cchaos

🎉 LGTM! It'd be great to get @chandlerprall 's eyes to do a quick scan in case there were any edge cases missed.

CHANGELOG.md

kibanamachine · 2021-11-08T21:44:13Z

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

kibanamachine · 2021-11-09T17:23:30Z

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

chandlerprall

Tooltip delays

The tooltip delay on the default controls feels really long, and is mismatched with the delay on the example custom controls; while we can't control the delay on any real custom control, our examples should provide a solid, recommended template for copy&pasting.

Fullscreen gap

Not sure if this PR introduced it, but I don't see this in the most recent release; when in fullscreen mode there is a big gap between the header & first row.

src/components/datagrid/controls/data_grid_toolbar.test.tsx

src/components/datagrid/controls/data_grid_toolbar.tsx

cchaos · 2021-11-09T18:55:26Z

Those tooltip delays were specifically designed as-is for the following reasons:

Internal controls (display & full screen) are repetitive and consistent throughout all data grid implementations which means users will only need to learn a few times then the tooltip becomes more of an annoyance, hence the long delay.
Custom controls are, well, custom and therefore are not widely used/known and therefore the shorter delay is helpful to quickly know what these custom controls do.

cee-chen · 2021-11-09T23:27:57Z

@chandlerprall re:

Fullscreen gap
Not sure if this PR introduced it, but I don't see this in the most recent release; when in fullscreen mode there is a big gap between the header & first row.

It looks like this is happening in latest main right now (but not in prod) - can you repro? If so I'd prefer to fix in a separate PR since it's unlikely this feature branch will land before the next release

EDIT: Separate PR with fix: #5369

kibanamachine · 2021-11-09T23:37:41Z

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

- Class was removed in a previous commit by Caroline

kibanamachine · 2021-11-10T18:46:00Z

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

chandlerprall

LGTM!

…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 <549577+cchaos@users.noreply.github.com> Co-authored-by: Chandler Prall <chandler.prall@gmail.com>

This PR fixes an issue reported in <elastic#122231> where the alerts table's `Full screen` button, recently [moved to the right side of `EuiDataGrid`](elastic/eui#5334) in [EUI 43.0.0](https://elastic.github.io/eui/#/package/changelog), overlapped the existing view selector. ### Details In the `8.0` release of the Security Solution, the alerts table `Full screen` button appears above the table on the **left**, per the screenshot below: ![8_0_alerts_table](https://user-images.githubusercontent.com/4459398/149236219-9aac04de-4bbb-4cef-8705-f6bb712fb19e.png) _Above: The alerts table `Full screen` button in `8.0`_ Starting with `8.1` (via [EUI 43.0.0](https://elastic.github.io/eui/#/package/changelog)), `EuiDataGrid`'s `Full screen` button has been [moved to the right side of `EuiDataGrid`](elastic/eui#5334), per the screenshot below: ![data_grid_before_after](https://user-images.githubusercontent.com/4459398/149237831-61aa7a30-695e-48d8-b016-89a0738d4bd9.png) _Above: `EuiDataGrid`'s full screen icon has moved from left to right_ The new location of the `Full screen` button overlapped the existing alerts table view selector, per the `Before` screenshot below: #### Before ![overlapped image](https://user-images.githubusercontent.com/60252716/148024399-24106303-baef-46bf-ad03-c4b53d78bbe8.png) _Above: Overlapping icons reported in <https://github.com/elastic/kibana/issues/122231>_ This PR fixes the overlap, per the `After` screenshots below: #### After Chrome `97.0.4692.71`: ![after_chrome](https://user-images.githubusercontent.com/4459398/149239990-1039d659-67a9-4d09-a910-3f8bdfd179e4.png) Firefox `96.0`: ![after_firefox](https://user-images.githubusercontent.com/4459398/149239483-590108d8-b6db-4c87-a3e7-579fc33e98a5.png) Safari `15.2`: ![after_safari](https://user-images.githubusercontent.com/4459398/149239764-1751b89c-125b-44b8-b9b2-984b630e3925.png)

…122901) ## [Security Solution] Fixes alerts table `Full screen` button overlap This PR fixes an issue reported in <#122231> where the alerts table's `Full screen` button, recently [moved to the right side of `EuiDataGrid`](elastic/eui#5334) in [EUI 43.0.0](https://elastic.github.io/eui/#/package/changelog), overlapped the existing view selector. ### Details In the `8.0` release of the Security Solution, the alerts table `Full screen` button appears above the table on the **left**, per the screenshot below: ![8_0_alerts_table](https://user-images.githubusercontent.com/4459398/149236219-9aac04de-4bbb-4cef-8705-f6bb712fb19e.png) _Above: The alerts table `Full screen` button in `8.0`_ Starting with `8.1` (via [EUI 43.0.0](https://elastic.github.io/eui/#/package/changelog)), `EuiDataGrid`'s `Full screen` button has been [moved to the right side of `EuiDataGrid`](elastic/eui#5334), per the screenshot below: ![data_grid_before_after](https://user-images.githubusercontent.com/4459398/149237831-61aa7a30-695e-48d8-b016-89a0738d4bd9.png) _Above: `EuiDataGrid`'s full screen icon has moved from left to right_ The new location of the `Full screen` button overlapped the existing alerts table view selector, per the `Before` screenshot below: #### Before ![overlapped image](https://user-images.githubusercontent.com/60252716/148024399-24106303-baef-46bf-ad03-c4b53d78bbe8.png) _Above: Overlapping icons reported in <https://github.com/elastic/kibana/issues/122231>_ This PR fixes the overlap, per the `After` screenshots below: #### After Chrome `97.0.4692.71`: ![after_chrome](https://user-images.githubusercontent.com/4459398/149239990-1039d659-67a9-4d09-a910-3f8bdfd179e4.png) Firefox `96.0`: ![after_firefox](https://user-images.githubusercontent.com/4459398/149239483-590108d8-b6db-4c87-a3e7-579fc33e98a5.png) Safari `15.2`: ![after_safari](https://user-images.githubusercontent.com/4459398/149239764-1751b89c-125b-44b8-b9b2-984b630e3925.png)

cee-chen added 7 commits October 27, 2021 13:57

[Setup] Move toolbar CSS to own file

130c722

[Cleanup] DRY out repeated specific CSS class

52398d9

- naming was incorrect in the case of the style selector popover

Reorder toolbar per Caroline's mocks

fc2203f

- Columns & Sort on left, with additionalControls to the right of those (+ update prop types to indicate that) - Style (TBD naming) and Fullscreen to the right

Convert display buttons to icons only

8ddb5e2

+ tweak grid styles icon to match Caroline's mock + popover location

Convert density UI to a compressed EuiFormRow w/ new text/order per m…

e5871eb

…ocks - refactor densityOptions to const outside hook

Update test snapshots

2782596

Add changelog entry

84009b0

cee-chen requested a review from cchaos October 27, 2021 22:14

cchaos suggested changes Oct 28, 2021

View reviewed changes

cee-chen added 5 commits October 29, 2021 10:56

Merge branch 'master' into datagrid-toolbar

6d71251

[PR feedback] Revert location of additionalControls + fix props descr…

7b1158a

…iption

[PR feedback] CSS feedback

05a696d

[PR feature request] Change toolbar icon based on grid density

6e3ecce

[PR feedback][extra] Fix missing sort control on styling example

07ea746

NB: sorting functionality won't actually work/isn't hooked up, but this should at least allow the show/hide UI toggle to work as expected

updoot snaps

e4e5d95

cee-chen requested a review from cchaos November 1, 2021 20:40

Merge branch 'master' into datagrid-toolbar

6611bf5

Merge branch 'master' into datagrid-toolbar

2f38124

cchaos and others added 2 commits November 8, 2021 12:30

Update CSS class to be more specific

4747d09

- It isn't necessary for popovers without drag & drop, name becomes verbose but it's hopefully worth it for clearer usage

cee-chen and others added 2 commits November 8, 2021 12:59

closes elastic#5092 - add changelog for fullscreen bugfix in Caroline…

99de8bc

…'s work

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

8ec3bd1

…to datagrid-toolbar

cchaos approved these changes Nov 8, 2021

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

cee-chen added 3 commits November 9, 2021 08:54

Merge branch 'main' into datagrid-toolbar

a8c12ed

[PR feedback] Changelog

9a4a5c7

Update snapshots

2c18e4d

chandlerprall suggested changes Nov 9, 2021

View reviewed changes

src/components/datagrid/controls/data_grid_toolbar.test.tsx Outdated Show resolved Hide resolved

src/components/datagrid/controls/data_grid_toolbar.tsx Outdated Show resolved Hide resolved

PR feedback

7abae68

cee-chen requested a review from chandlerprall November 10, 2021 16:20

Remove unused CSS class

921d9c2

- Class was removed in a previous commit by Caroline

chandlerprall approved these changes Nov 10, 2021

View reviewed changes

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

cee-chen deleted the datagrid-toolbar branch November 10, 2021 22:24

This was referenced Nov 19, 2021

[EuiDataGrid] Add extra left prepend position to the additionalControls API #5394

Merged

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

Merged

thompsongl mentioned this pull request Dec 9, 2021

Upgrade EUI to v43.1.1 elastic/kibana#120935

Merged

1 task

cee-chen mentioned this pull request Jan 5, 2022

[EuiDataGrid] Full screen mode can sometimes be quirky with adding the body class #5092

Closed

andrew-goldstein mentioned this pull request Jan 13, 2022

[Security Solution] Fixes alerts table Full screen button overlap elastic/kibana#122901

Merged

[EuiDataGrid] Toolbar UI layout reorganization #5334

[EuiDataGrid] Toolbar UI layout reorganization #5334

Uh oh!

Conversation

cee-chen commented Oct 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Before

After

Checklist

Uh oh!

cee-chen commented Oct 27, 2021

Uh oh!

kibanamachine commented Oct 27, 2021

Uh oh!

cchaos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cee-chen commented Oct 29, 2021

Uh oh!

kibanamachine commented Oct 29, 2021

Uh oh!

kibanamachine commented Oct 29, 2021

Uh oh!

snide commented Nov 1, 2021

Uh oh!

weltenwort commented Nov 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cee-chen commented Nov 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cchaos commented Nov 1, 2021

Uh oh!

cee-chen commented Nov 1, 2021

Uh oh!

kibanamachine commented Nov 1, 2021

Uh oh!

cchaos commented Nov 2, 2021

Uh oh!

cee-chen commented Nov 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cchaos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kibanamachine commented Nov 8, 2021

Uh oh!

kibanamachine commented Nov 9, 2021

Uh oh!

chandlerprall left a comment

Choose a reason for hiding this comment

Tooltip delays

Fullscreen gap

Uh oh!

Uh oh!

Uh oh!

cchaos commented Nov 9, 2021

Uh oh!

cee-chen commented Nov 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kibanamachine commented Nov 9, 2021

Uh oh!

kibanamachine commented Nov 10, 2021

Uh oh!

chandlerprall left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

cee-chen commented Oct 27, 2021 •

edited

Loading

weltenwort commented Nov 1, 2021 •

edited

Loading

cee-chen commented Nov 1, 2021 •

edited

Loading

cee-chen commented Nov 8, 2021 •

edited

Loading

cee-chen commented Nov 9, 2021 •

edited

Loading