Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 9 commits into from
Jan 27, 2022

Conversation

cee-chen
Copy link
Member

@cee-chen cee-chen commented Jan 25, 2022

Summary

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

  1. Visually I think we need more borders especially around the scrollbars
    image

Before

After

Chrome - visible vertical scrollbar only

Screen Shot 2022-01-25 at 3 51 01 PM

Chrome - visible horizontal scrollbar only

Chrome - both scrollbars visible

Firefox - inline scrollbars

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

- [ ] Checked for breaking changes and labeled appropriately

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

Comment on lines +214 to +227
<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>
Copy link
Member Author

@cee-chen cee-chen Jan 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link

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

@snide
Copy link
Contributor

snide commented Jan 26, 2022

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.

image

@cee-chen
Copy link
Member Author

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
Copy link
Member Author

cee-chen commented Jan 26, 2022

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
Copy link

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

Copy link
Contributor

@miukimiu miukimiu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

data-grid-overflow@2x

- By converting the pagination border to a box-shadow and tweaking z-indices to better reflect overlapping behavior
@cee-chen
Copy link
Member Author

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
Copy link
Member Author

cee-chen commented Jan 27, 2022

@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.

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :shipit:

@kibanamachine
Copy link

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
Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

@kibanamachine
Copy link

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

@cee-chen
Copy link
Member Author

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

@cee-chen cee-chen merged commit ce1dfa8 into elastic:main Jan 27, 2022
@cee-chen cee-chen deleted the datagrid/4458/scrolling branch January 27, 2022 21:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants