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

Fix rendering performance bottleneck in carousel #2281

Open
wants to merge 1 commit into
base: next
Choose a base branch
from

Conversation

patrick-mcdougle
Copy link

@patrick-mcdougle patrick-mcdougle commented Nov 22, 2024

Fixes #2280

Before

Screenshot 2024-11-22 at 4 00 41 PM

After

Screenshot 2024-11-22 at 4 03 14 PM

Notice how none of the carousels on the right side have any synchronous Layout anymore, but the browser does one big layout once in the requestAnimationFrame callback.

NOTE: The scales on these screenshots are not the same...but the key point to notice is there there aren't as many tiny red dog-ears on each of the individual carousels which is the concern I was trying to fix.

Copy link

vercel bot commented Nov 22, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
shoelace ✅ Ready (Inspect) Visit Preview Nov 22, 2024 10:36pm

@patrick-mcdougle
Copy link
Author

I did some light testing in chrome by using the override feature and it seemed to work alright, but I must warn, this will convert something that used to be synchronous to be async, so there might be some assumptions hiding somewhere which could be the source of bugs.

Someone (and I could be willing to do this), should probably audit all of the callers of goToSlide to see if any of the code after it's called could possibly expect that the actual scrolling has been completed. Biggest likely offender is that state property this.pendingSlideChange which would now be guaranteed to still be true in this frame, but not in this task/tick of the event loop.

@claviska
Copy link
Member

@alenaksu any concerns before we merge this?

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.

sl-carousel goToSlide has synchronous layout
2 participants