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

[EuiToolTip] Improve render performance #3596

Merged
merged 8 commits into from
Jun 15, 2020

Conversation

thompsongl
Copy link
Contributor

@thompsongl thompsongl commented Jun 11, 2020

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

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

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

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

@thompsongl thompsongl marked this pull request as ready for review June 15, 2020 19:42
@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

Copy link
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.

src/global_styling/mixins/_tool_tip.scss Outdated Show resolved Hide resolved
src/components/tool_tip/tool_tip.tsx Outdated Show resolved Hide resolved
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, didn't see anything broken

@kibanamachine
Copy link

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

Copy link
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
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.

[EuiTooltip] Performance issues
4 participants