Skip to content

Conversation

@cee-chen
Copy link
Contributor

@cee-chen cee-chen commented Nov 9, 2021

Summary

Before

before

After

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

Skipping changelog entry since this bug was caused in recent main and should not have hit an actual release

@cee-chen cee-chen added skip-changelog Use on PRs to skip changelog requirement (Don't delete - used for automation) bug labels Nov 9, 2021
@cee-chen cee-chen marked this pull request as ready for review November 9, 2021 23:52
@kibanamachine
Copy link

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

- unfortunately this requires passing headerRowHeight via prop instead of via context, since useContext in a shallow rendering scenario is super difficult to mock
- not the worst thing in the world since we're already passing so many props
@kibanamachine
Copy link

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

@chandlerprall
Copy link
Contributor

I was curious why the top positioning is affected by fullscreen or not, and it turns out the cells are positioned relative to the virtualized container until full screen is enabled, then they're positioned relative to their row element.

cell_offset_parent_fullscreen.mov

Looks like the will-change from https://github.com/elastic/eui/blob/main/src/components/datagrid/_data_grid.scss#L25-L27 - added by #3726 - is responsible because transform creates a new stacking context. The will-change attribute was added to solve a performance issue in full screen that doesn't appear to exist anymore, and the css looks like it can be safely removed, keeping the style.top calculation the same for both fullscreen & non-fullscreen experiences.

@cee-chen
Copy link
Contributor Author

Whoa! TIL that will-change will do that.

The will-change attribute was added to solve a performance issue in full screen that doesn't appear to exist anymore,

Just to clarify, this doesn't exist anymore because we switched to virtualized rendering - is that correct?

This reverts commit 04ef091.

This reverts commit a236377
- should no longer be necessary now that virtualization is being used, and was creating top positioning issues
@cee-chen
Copy link
Contributor Author

@chandlerprall Superior fix has been pushed up :)

@kibanamachine
Copy link

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

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.

LGTM! Tested locally & in the published docs; don't forget a changelog! changelog not necessary because the bug didn't get released

Just to clarify, this doesn't exist anymore because we switched to virtualized rendering - is that correct?

I don't remember enough of the context from when the performance issue was discovered, but I believe the lag was from a combination of the cell-hover performance problems (#5136 + #5163) plus the non-virtualized grid having significant re-layouts.

@cee-chen cee-chen merged commit 9339064 into elastic:main Nov 10, 2021
@cee-chen cee-chen deleted the datagrid-fullscreen-fix branch November 10, 2021 22:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-changelog Use on PRs to skip changelog requirement (Don't delete - used for automation)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants