Skip to content

perf: lazy tooltip#2141

Merged
bennypowers merged 6 commits intomainfrom
perf/tooltip/lazy
Sep 7, 2022
Merged

perf: lazy tooltip#2141
bennypowers merged 6 commits intomainfrom
perf/tooltip/lazy

Conversation

@bennypowers
Copy link
Copy Markdown
Member

@bennypowers bennypowers commented Sep 7, 2022 •
edited
Loading

What I did

I identified a performance issue with tooltip, which blocks the main thread for about 200ms for each tooltip added to the dom. This PR defers initializing popperjs until the tooltip is first show. Rendering 3000 tooltip elements on a page went from 40s-180s to ~5s.

This should be considered a hotfix patch until we can replace popper with something more modern: floating-ui or our own homebrew.

Testing Instructions

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Sep 7, 2022 •
edited
Loading

🦋 Changeset detected

Latest commit: 7528bca

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@patternfly/pfe-core Minor
@patternfly/pfe-tooltip Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@bennypowers
Copy link
Copy Markdown
Member Author

bennypowers commented Sep 7, 2022

depends on #2140

@github-actions github-actions Bot added demo Updating demo pages functionality Functionality, typically pertaining to the JavaScript. styles An issue or PR pertaining only to CSS/Sass labels Sep 7, 2022
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Sep 7, 2022 •
edited
Loading

✅ Deploy Preview for patternfly-elements ready!

Name Link
🔨 Latest commit fcc05ef
😎 Deploy Preview https://deploy-preview-2141--patternfly-elements.netlify.app/

_To edit notification comments on pull requests, go to your Netlify site settings.

@github-actions github-actions Bot added the AT passed Automated testing has passed label Sep 7, 2022
@bennypowers bennypowers enabled auto-merge (squash) September 7, 2022 13:50
brianferry
brianferry approved these changes Sep 7, 2022
View reviewed changes
Copy link
Copy Markdown
Collaborator

@brianferry brianferry left a comment

Choose a reason for hiding this comment

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

Looks good to me, definitely noticing the improvements locally. Took around 10s to run pre commits and afterwards less than a second.

@bennypowers bennypowers merged commit 166ecee into main Sep 7, 2022
@bennypowers bennypowers deleted the perf/tooltip/lazy branch September 7, 2022 14:17
@brianferry
Copy link
Copy Markdown
Collaborator

brianferry commented Sep 7, 2022

Only thing I noticed was the performance demo on the preview doesn't look like it can find the module for lit, is this an issue with sub-categories of demos right now @bennypowers?

@bennypowers
Copy link
Copy Markdown
Member Author

bennypowers commented Sep 7, 2022

yeah something fishy is going on: view-source:https://deploy-preview-2141--patternfly-elements.netlify.app/components/tooltip/demo/performance

I'm not satisfied with the demo stuff. Might be best to require demos to be demo-slug/index.html etc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@brianferry brianferry brianferry approved these changes

Assignees

No one assigned

Labels

AT passed Automated testing has passed demo Updating demo pages functionality Functionality, typically pertaining to the JavaScript. ready to merge styles An issue or PR pertaining only to CSS/Sass

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@bennypowers @brianferry