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] Change data grid body background to light gray #5562

Merged
merged 5 commits into from
Jan 31, 2022

Conversation

cee-chen
Copy link
Member

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

Summary

This attempts to address item 4 in #4458:

  1. It would be great if while scrolling with virtualization which shows a white background while the cells load, we could have either add in some light gray like $euiPageBackgroundColor, or even some sort of pattern indication "loading".

I tried to take some screencaps but the GIFs didn't end up capturing the very subtle color change sufficiently in terms of either FPS or color swatching. It's probably easier just to compare staging vs. prod directly:

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

@@ -30,7 +30,7 @@
width: 100%;
overflow: hidden;
z-index: 2; // Sits above the pagination below it, but below the controls above it
background: $euiColorEmptyShade;
background: $euiPageBackgroundColor;
Copy link
Member Author

@cee-chen cee-chen Jan 25, 2022

Choose a reason for hiding this comment

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

@miukimiu I'm not totally sure if we still even want this change or feedback item per #4458 and would appreciate your awesome designer eyes here! https://eui.elastic.co/pr_5562/#/tabular-content/data-grid-virtualization to test the change

@@ -49,14 +49,6 @@
}
}


.euiDataGrid__controlScroll {
Copy link
Member Author

Choose a reason for hiding this comment

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

This was a misc extra cleanup, I noticed that this belonged better in the toolbar CSS and not the overall datagrid and decided to move it.

@kibanamachine
Copy link

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

Copy link
Contributor

@elizabetdev elizabetdev left a comment

Choose a reason for hiding this comment

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

@constancecchen I think the$euiPageBackgroundColor (2) works better than the white background (1).

But ideally, it would be great if we could have some placeholders (3) similar to EuiLoadingContent. But I'm not sure if this is possible... So, if not, I'm ok with the current solution.

Also, once you merge master we will have borders like on the bellow images. Is that correct?

datagrid-loading@2x

@cee-chen
Copy link
Member Author

cee-chen commented Jan 28, 2022

Unfortunately I'm not sure the placeholder cells are really possible. Cell widths and heights come in from react-window via JS and there currently isn't a super easy way to get that into a repeatable CSS gradient (until we finalize css-in-JS, perhaps), and we wouldn't want to use JS to do it due to performance considerations. I'm just not sure it's worth the level of effort essentially, especially when our virtualization library already does a fairly good job of rendering cells in time.

To answer your question, yes # 2 is what main will look like once this PR merges.

EDIT: It's also worth noting that this will currently affect full screen mode when the # of rows is not high enough to scroll - this can be tested on https://eui.elastic.co/pr_5562/#/tabular-content/data-grid

@cee-chen
Copy link
Member Author

Heya @miukimiu! Just wanted to check in on whether you think this is OK to merge this week. Thanks!

Copy link
Contributor

@elizabetdev elizabetdev left a comment

Choose a reason for hiding this comment

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

Yes, let's merge it! 🎉

@cee-chen cee-chen enabled auto-merge (squash) January 31, 2022 17:24
@kibanamachine
Copy link

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

@cee-chen cee-chen merged commit 7412e4f into elastic:main Jan 31, 2022
@cee-chen cee-chen deleted the datagrid/4458/background-color branch January 31, 2022 18:09
cee-chen added a commit to cee-chen/eui that referenced this pull request Feb 7, 2022
- cells no longer have a set background color (to allow row color to bleed through), and we were previously relying on the grid body's bg color to be white
cee-chen pushed a commit that referenced this pull request Feb 10, 2022
* Pass positioning and height/width to portalled rows

+ adjust scrolling workaround to obtain offsetTop from row parent instead of cell, now that individual cells have a top: 0

* Update snapshots

* Fix scrolling workaround to use row offsetTop

- now that cells are relative to the row parent, their offsetTop is 0

* Simplify row CSS

- Remove `@include euiDataGridRowCell` mixin and target the row directly

- Remove isStripableRow cell logic and instead use CSS nth-child for alternating stripes

- Swap stripes and highlight CSS so that highlight takes precendence over stripes without an !important

- Remove background color on individual cells to allow row colors to show through (NB: grid body is already set to the correct background color as well)

* Add unit tests for row manager

- skipping the mutation observer for now

* Account for #5562

- cells no longer have a set background color (to allow row color to bleed through), and we were previously relying on the grid body's bg color to be white

* Fix row widths bugging out due to stale containerRef reference

- using CSS `left`/`right` and `relative` on the inner grid parent solves the issue instead, as the row is now always correct width

* Add `data-gridrow-index` attr for users to more easily hook into rows

* Fix rows to add `--striped` class instead of CSS :nth-child

- which does not work well due to children changing on virtualization

+ set additional visible row index data attribute for extra detail

+ use dataset

* Fix striped footer rows

- was previously not working on prod, and now it does

* [PR feedback] Convert getRow args to an object
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.

3 participants