Skip to content

[EuiDataGrid] Fix full screen display bug#5369

Merged
cee-chen merged 4 commits intoelastic:mainfrom
cee-chen:datagrid-fullscreen-fix
Nov 10, 2021
Merged

[EuiDataGrid] Fix full screen display bug#5369
cee-chen merged 4 commits intoelastic:mainfrom
cee-chen:datagrid-fullscreen-fix

Conversation

@cee-chen
Copy link
Copy Markdown
Contributor

@cee-chen cee-chen commented Nov 9, 2021 •
edited
Loading

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 mentioned this pull request Nov 9, 2021
Merged
6 tasks
@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
Copy Markdown

kibanamachine commented Nov 10, 2021

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

@cee-chen
Add unit tests
04ef091 
- 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
Copy Markdown

kibanamachine commented Nov 10, 2021

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

@chandlerprall
Copy link
Copy Markdown
Contributor

chandlerprall commented Nov 10, 2021

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
Copy Markdown
Contributor Author

cee-chen commented Nov 10, 2021

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?

@cee-chen
Revert approach
ff42391 
This reverts commit 04ef091.

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

cee-chen commented Nov 10, 2021

@chandlerprall Superior fix has been pushed up :)

@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Nov 10, 2021

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

chandlerprall
chandlerprall approved these changes Nov 10, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@chandlerprall chandlerprall left a comment •
edited
Loading

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

Reviewers

1 more reviewer

@chandlerprall chandlerprall chandlerprall approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@cee-chen @kibanamachine @chandlerprall