[EuiDataGrid] DRY out scrolling/scrollbar detections and add scroll border overlays #5563

cee-chen · 2022-01-25T23:49:16Z

Summary

The first 3 commits are DRY cleanup, and the 4th one attempts to address item 3 in #4458:

Visually I think we need more borders especially around the scrollbars

Before

After

Chrome - visible vertical scrollbar only

Chrome - visible horizontal scrollbar only

Chrome - both scrollbars visible

Firefox - inline scrollbars

Checklist

Check against all themes for compatibility in both light and dark modes
Checked in mobile
Checked in Chrome, Safari, Edge, and Firefox
Checked on Windows OS scrollbars

- [ ] 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

- to DRY out various concerns that account for scrolling & scroll bars

cee-chen · 2022-01-26T00:05:57Z

src/components/datagrid/utils/scrolling.tsx

+      <div className="euiDataGrid__scrollOverlay" role="presentation">
+        {scrollBarHeight > 0 && (
+          <div
+            className="euiDataGrid__scrollBarOverlayBottom"
+            style={{ bottom: scrollBarHeight, right: 0 }}
+          />
+        )}
+        {scrollBarWidth > 0 && (
+          <div
+            className="euiDataGrid__scrollBarOverlayRight"
+            style={{ bottom: scrollBarHeight, right: scrollBarWidth }}
+          />
+        )}
+      </div>


Just want to chat a little bit about these extra divs:

Why an absolutely positioned overlay div on top of the existing grid?
Adding a border to .euiDataGrid__virtualized itself changes the actual containing size of its contents and causes double borders (the grid wrapper has a border and the cells have a border). I needed a border that would sit on top of the existing cell borders and overlap them, not duplicate them.
The other option we could have gone with is trying to detect which cells butt up against a grid wrapper border and conditionally removing the borders on those, but I saw that approach as far more tedious than this one.

Could this have been accomplished with :after/:before pseudo selectors instead of 3 divs?
For the main .euiDataGrid__scrollOverlay div, yes, but for the other 2 divs that handle the scrollbar borders, no. The main issue is that JS can know the OS-set width/height of scrollbars but CSS cannot. The attr() selector is not ready for things like setting height/widths, although this may possibly be cleaned up/solved when we switch to emotion/CSS-in-JS.
The other option I could have gone with is trying to nest the absolutely positioned scrollbar border divs inside .euiDataGrid__virtualized which would then respect scrollbar width/heights, but I see this approach as the lesser of 2 evils because it doesn't potentially create side effects inside the grid contents, and also puts all div cruft in 1 easy-to-find place and adds role="presentation" to ensure maximum accessibility.

Why not use CSS gradients to mimic borders?
Same issue as above (no way to get the scroll heights/widths into CSS), although I think it's worth trying once we can get JS vars into CSS. And yes, I know it's hilarious I'm mimicking borders in every possible way in this PR except actually setting border 💀

kibanamachine · 2022-01-26T00:26:25Z

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

snide · 2022-01-26T16:40:12Z

Filling in for @cchaos 😸

Pretty clever. I think when virtualization is used 90% of the time the grid will be included within a panel that is handling the borders (Discover is this way, dashboard panels from Lens are this way...etc). It'll be similar to these examples in the docs. I think we'll want to provide props or something to control the left/right bordering so that we don't run into double borders.

Maybe we just say folks need to write custom css for this (I've certainly done something like that before), but we might want to consider it and at least update our docs to show the correct usage.

When not within a panel, the affordance you suggest is fine and likely preferred.

cee-chen · 2022-01-26T17:30:13Z

Ahh, I totally forgot about gridStyles.border! This is easy enough to account for, I'll push up a fix. Thanks a million for the eagle eyes Dave!

cee-chen · 2022-01-26T17:57:52Z

Alrighty, the scrolling overlay should now respect gridStyle.border: 'horizontal' and none (60eac2e):

I left the vertical border on the vertical scrollbar on horizontal as I felt it still added appropriate context. LMK if you disagree however!

kibanamachine · 2022-01-26T18:29:38Z

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

miukimiu

Thanks @constancecchen,

Tested in Chrome, Safari, Firefox and Edge and LGTM! 🎉

Just one small detail. When we are in full screen and there is an horizontal scrollbar, it would look better if the border between the pagination and scrollbar look like it has 1px instead of 2px.

- By converting the pagination border to a box-shadow and tweaking z-indices to better reflect overlapping behavior

cee-chen · 2022-01-27T16:56:21Z

Awesome catch, thank you for the amazing QA!! The fix for that is in 612ae0e. I tested it in both Chrome and Firefox in fullscreen mode with non-scrolling content, vertically scrolling content and horizontally scrolling content.

cee-chen · 2022-01-27T16:58:24Z

@chandlerprall Do you mind quickly reviewing the first 3 commits of this PR? It involves DRYing out JS scrolling logic that we previously discussed in #5515 (comment), and I just want to make sure you're good with it before merging.

chandlerprall

Code refactor LGTM, glanced at but did not look into SCSS changes. I think this would be improved with a couple more test cases, but otherwise

src/components/datagrid/utils/scrolling.test.tsx

kibanamachine · 2022-01-27T17:31:48Z

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

- make height/width comparisons more explicit, make mockOuterGrid less opinionated - add both true/false comparisons for hasVerticalScroll and hasHorizontalScroll

chandlerprall

Looks great!

kibanamachine · 2022-01-27T18:27:26Z

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

cee-chen · 2022-01-27T21:45:20Z

Wahoo! Thanks y'all, this is a pretty jazzy enhancement!

cee-chen added 5 commits January 25, 2022 15:18

Add new useScrollBars helper

2b9429d

- to DRY out various concerns that account for scrolling & scroll bars

DRY out useUnconstrainedHeight's scrollbar height calculation

18ee38e

DRY out scrollCellIntoView's grid scrolling check

23a776c

Add borders for scrolling grids and grids with scrollbars

8f4cda9

Add changelog entry

2655cf5

cee-chen requested review from chandlerprall and miukimiu January 25, 2022 23:49

cee-chen commented Jan 26, 2022

View reviewed changes

cee-chen added 2 commits January 26, 2022 09:45

Correctly account for gridStyle.border

60eac2e

[misc] Ensure EuiPanel examples properly clip on rounded borders

f7a7eba

miukimiu approved these changes Jan 27, 2022

View reviewed changes

[PR feedback] Fix fullscreen mode scroll overlay duplication

612ae0e

- By converting the pagination border to a box-shadow and tweaking z-indices to better reflect overlapping behavior

chandlerprall suggested changes Jan 27, 2022

View reviewed changes

src/components/datagrid/utils/scrolling.test.tsx Show resolved Hide resolved

[PR feedback] Improve useScrollBars unit tests

34a531a

- make height/width comparisons more explicit, make mockOuterGrid less opinionated - add both true/false comparisons for hasVerticalScroll and hasHorizontalScroll

chandlerprall approved these changes Jan 27, 2022

View reviewed changes

cee-chen merged commit ce1dfa8 into elastic:main Jan 27, 2022

cee-chen deleted the datagrid/4458/scrolling branch January 27, 2022 21:45

cee-chen mentioned this pull request Jan 27, 2022

[EuiDataGrid] UX Cleanups #4458

Closed

breehall mentioned this pull request Feb 8, 2022

Upgrade EUI to v48.1.1 elastic/kibana#125023

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EuiDataGrid] DRY out scrolling/scrollbar detections and add scroll border overlays #5563

[EuiDataGrid] DRY out scrolling/scrollbar detections and add scroll border overlays #5563

cee-chen commented Jan 25, 2022 •

edited

Loading

cee-chen Jan 26, 2022 •

edited

Loading

kibanamachine commented Jan 26, 2022

snide commented Jan 26, 2022

cee-chen commented Jan 26, 2022

cee-chen commented Jan 26, 2022 •

edited

Loading

kibanamachine commented Jan 26, 2022

miukimiu left a comment

cee-chen commented Jan 27, 2022

cee-chen commented Jan 27, 2022 •

edited

Loading

chandlerprall left a comment

kibanamachine commented Jan 27, 2022

chandlerprall left a comment

kibanamachine commented Jan 27, 2022

cee-chen commented Jan 27, 2022

[EuiDataGrid] DRY out scrolling/scrollbar detections and add scroll border overlays #5563

[EuiDataGrid] DRY out scrolling/scrollbar detections and add scroll border overlays #5563

Conversation

cee-chen commented Jan 25, 2022 • edited Loading

Summary

Before

After

Chrome - visible vertical scrollbar only

Chrome - visible horizontal scrollbar only

Chrome - both scrollbars visible

Firefox - inline scrollbars

Checklist

cee-chen Jan 26, 2022 • edited Loading

Choose a reason for hiding this comment

kibanamachine commented Jan 26, 2022

snide commented Jan 26, 2022

cee-chen commented Jan 26, 2022

cee-chen commented Jan 26, 2022 • edited Loading

kibanamachine commented Jan 26, 2022

miukimiu left a comment

Choose a reason for hiding this comment

cee-chen commented Jan 27, 2022

cee-chen commented Jan 27, 2022 • edited Loading

chandlerprall left a comment

Choose a reason for hiding this comment

kibanamachine commented Jan 27, 2022

chandlerprall left a comment

Choose a reason for hiding this comment

kibanamachine commented Jan 27, 2022

cee-chen commented Jan 27, 2022

cee-chen commented Jan 25, 2022 •

edited

Loading

cee-chen Jan 26, 2022 •

edited

Loading

cee-chen commented Jan 26, 2022 •

edited

Loading

cee-chen commented Jan 27, 2022 •

edited

Loading