Repository navigation
perf: optimizations - #35
Open
petrkonecny2 wants to merge 3 commits into
Open
petrkonecny2 wants to merge 3 commits into
petrkonecny2 wants to merge 3 commits into
Conversation
interpolateInsideCarousel built and searched one point per slide on every frame, for every slide using it, so a long carousel got slower the more slides it had. Only the points around the slide and around its clone change the result, so it builds those alone. The result is identical, which a test checks against the old all-slides version. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The pan activated only after 30pt of movement in any direction, but onUpdate subtracted the translation from where the finger first touched, so the row jumped by that distance when a drag started. The starting point now counts the movement made before activation. The pan now activates on horizontal movement only and fails on vertical movement, so scrolling a parent past the carousel no longer drags and snaps it. The gesture is memoized, and the adapter's drag callback is stable, so a re-render during a drag no longer hands the detector a new gesture. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Mounting slides as they entered the window happened during the snap animation, and the page tracking behind it re-rendered the carousel on every page change, even without a window. Building the new slide's views mid-animation made swiping visibly stutter, which cost more than keeping every slide mounted saved. Every slide stays mounted again. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Performance and smoothness fixes found while testing the carousel in the Ascension app on a device. One commit each.
perf: interpolate over the points near the slide only.
interpolateInsideCarousel, behinduseInterpolateInsideCarousel, built and searched one point per slide on every frame, for every slide using it. A long carousel got slower with every slide it had. Only the points around the slide, and around its clone with infinite scroll, change the result. Every other point sits on a flat stretch between two of those, so the helper now builds at most six points. The output is identical: a new test checks it against the previous all-slides version across 2 to 12 slides, three value sets including an offset, and both infinite modes.fix: stop the row jumping when a drag starts or the carousel re-renders. Three problems in
AnimatedPagedView:onUpdatesubtractedtranslationX, which is measured from where the finger first touched down. The row jumped by about 30pt on activation. The starting point now includes the movement made before activation.minDistancecounts movement in any direction, so scrolling a parent past the carousel could grab it, shift it sideways and snap it when the parent took the gesture back. The pan now usesactiveOffsetX([-10, 10])andfailOffsetY([-10, 10]), so it only starts on horizontal movement and gives way to vertical scrolling.Gesture.Pan()was created on every render, and the carousel re-renders while it's dragged. It's now memoized, and the adapter'sonScrollBeginDragis stable so the memo holds.perf: remove
renderWindow. Added in #32, it unmounted slides far from the current one. On a device it made swiping worse:Every slide stays mounted again. Removing it also removes
isInRenderWindowand its tests, and the README row. This is a breaking change for anyone who passesrenderWindow, but it only shipped in 0.1.15, today.Testing:
type:check,lint,test(38 passing) and Prettier. In the Ascension app, all three changes were patched in together. On a phone, swiping was choppy withrenderWindowand smooth without it. On the iOS simulator, swipes still page and land centred. The drag-start jump fix follows from the code, but nobody has compared it side by side on a device.Related Tickets/Issues
Follow-up to #32 and #34.
🤖 Generated with Claude Code
📦 Published PR as canary version:
0.1.18--canary.35.f944505.0✨ Test out this PR locally via:
npm install @strv/react-native-hero-carousel@0.1.18--canary.35.f944505.0 # or yarn add @strv/react-native-hero-carousel@0.1.18--canary.35.f944505.0