-
Notifications
You must be signed in to change notification settings - Fork 842
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
[DataGrid] Add will-change to CSS to improve performance in chrome full screen mode #3726
[DataGrid] Add will-change to CSS to improve performance in chrome full screen mode #3726
Conversation
Preview documentation changes for this PR: https://eui.elastic.co/pr_3726/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_3726/ |
I still think that the most desirable thing to optimize the data grid performance is virtualized scrolling. I bet if we apply it to the data grid, we'll get rid of such a hover problem, therefore the |
I do agree, that virtualize scrolling should be the best solution, but the performance lag in Chrome on a retina screen justifies a in between solution IMO. I've adapted the CSS to apply will-change only on the transforms of the row. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, reading through the spec, it seems that adding this directly to the stylesheet is not optimal. That, instead, the addition of this property should be handled via JS in anticipation of the change, not a blanket change. (See first example in the spec link).
ok, agreed, will implement the JS approach |
I've tried to add the property directly but in this case it doesn't help, that was the draft |
Thanks for looking into that solution. Honestly, I'd rather just wait for virtualization rather than throw in a rule that could be more detrimental to the overall browser performance when DataGrid's don't exist. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I will walk back my previous statement after reviewing your before and after screenshots again. There is quite an improvement being made. Let's go ahead and get this merged but, @chandlerprall can you be sure to test your virtualization effort without this?
The PR will need to be rebased with master to get the lastest changes to the changelog.
800d0c0
to
4ef681b
Compare
thx @cchaos, so I can also finish #3668 which made the performance worse in this case. had to force push, because rebase messed up the GitHub commit history, and no I wanted to adapt the Changelog, but the linter complains because of 293 errors, that seem to be unrelated to my 1 line of Changelog add-on 😄. how should I fix this? |
Preview documentation changes for this PR: https://eui.elastic.co/pr_3726/ |
ok, seemed to be a eslint cache issue that has been resolved locally be upgrading node. I could commit the changelog entry |
Preview documentation changes for this PR: https://eui.elastic.co/pr_3726/ |
Summary
Significant improvement of performance issues encountered in #3705. Screens taken using Chrome 83, MacBook Pro 15'' 2019 on a retina screen, rendering 100 rows.
Before:
After:
Important: pls test on a retina/high resolution monitor, there the lag is significant, and also the improvement
Checklist
- [ ] Check against all themes for compatibility in both light and dark modes- [ ] Checked in mobile- [ ] Props have proper autodocs- [ ] Added documentation- [ ] Checked Code Sandbox works for the any docs examples- [ ] Added or updated jest tests- [ ] Checked for breaking changes and labeled appropriately- [ ] Checked for accessibility including keyboard-only and screenreader modes