Skip to content

[EuiToolTip] Improve render performance#3596

Merged
thompsongl merged 8 commits intoelastic:masterfrom
thompsongl:tooltip-perf
Jun 15, 2020
Merged

[EuiToolTip] Improve render performance#3596
thompsongl merged 8 commits intoelastic:masterfrom
thompsongl:tooltip-perf

Conversation

@thompsongl
Copy link
Copy Markdown
Contributor

@thompsongl thompsongl commented Jun 11, 2020 •
edited
Loading

Summary

Resolves #3568 by 1) moving the animation delay from CSS to JS and using it to delay subcomponent mounting and 2) using the enqueueStateChange utility to batch EuiToolTip visibility state updates that result in DOM node add/remove updates.
Secondary improvement was to remove the EuiToolTipPopover initialization call to updateDimensions, as the nested EuiResizeObserver component also calls the method on mount.

Checklist

- [ ] Check against all themes for compatibility in both light and dark modes
- [ ] Checked in mobile
- [ ] Checked in IE11 and Firefox
- [ ] 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
  • A changelog entry exists and is marked appropriately

@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Jun 11, 2020

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

@thompsongl thompsongl marked this pull request as draft June 11, 2020 21:13
@thompsongl
Copy link
Copy Markdown
Contributor Author

thompsongl commented Jun 11, 2020

Pulled this back to draft because, although there are definite improvements, CPU usage in the Kibana issue use case still remains high.
Rough Chrome CPU usage benchmarking with heavy mouseover interaction in Discover:
title tooltip: ~25%
New EuiToolTip: ~80%
Old EuiToolTip: ~95%

@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Jun 11, 2020

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

@thompsongl thompsongl requested a review from chandlerprall June 15, 2020 19:42
@thompsongl thompsongl marked this pull request as ready for review June 15, 2020 19:42
snide
snide reviewed Jun 15, 2020
View reviewed changes
chandlerprall
chandlerprall reviewed Jun 15, 2020
View reviewed changes
@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Jun 15, 2020

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

@thompsongl thompsongl mentioned this pull request Jun 15, 2020
Closed
@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Jun 15, 2020

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

snide
snide reviewed Jun 15, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

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

Tested in browser and double checked accessibility still works. I think it's worthwhile to clean up that sass function. Noticed some other small bits, otherwise LGTM on changes.

chandlerprall
chandlerprall approved these changes Jun 15, 2020
View reviewed changes
Copy link
Copy Markdown
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, didn't see anything broken

@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Jun 15, 2020

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

snide
snide approved these changes Jun 15, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

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

Thanks for the changes! LGTM

@thompsongl thompsongl merged commit b67f644 into elastic:master Jun 15, 2020
@thompsongl thompsongl mentioned this pull request Jun 29, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@chandlerprall chandlerprall chandlerprall approved these changes

@snide snide snide approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[EuiTooltip] Performance issues

4 participants

@thompsongl @kibanamachine @chandlerprall @snide