meliorence / react-native-snap-carousel

Swiper/carousel component for React Native featuring previews, multiple layouts, parallax images, performant handling of huge numbers of items, and more. Compatible with Android & iOS.
BSD 3-Clause "New" or "Revised" License
10.37k stars 2.29k forks source link

firstItem doesn't work reliably #538

Open thekevinbrown opened 5 years ago

thekevinbrown commented 5 years ago

Is this a bug report, a feature request, or a question?

Bug report

Have you followed the required steps before opening a bug report?

(Check the step you've followed - put an x character between the square brackets ([]).)

Have you made sure that it wasn't a React Native bug?

Yes

Is the bug specific to iOS or Android? Or can it be reproduced on both platforms?

It's reproducible on both platforms but has slightly different behaviour on each. On iOS index 14 is shown, on Android, index 6 is shown, when firstItem is 30.

Is the bug reproducible in a production environment (not a debug one)?

Yes

Environment

Here's a snack which reproduces the behaviour: https://snack.expo.io/@thekevinbrown/snap-carousel-first-item-bug

That environment's details: react-native-snap-carousel: 3.8.0 Expo v33.0.0

Expected Behavior

Expected carousel to load with a green screen focused (slide index 30)

Actual Behavior

Carousel loads with slide index 14 focused or 6 focused depending on platform.

Reproducible Demo

https://snack.expo.io/@thekevinbrown/snap-carousel-first-item-bug

Steps to Reproduce

  1. Load more than a few pages of any type of slide into snap-carousel
  2. Set firstItem to a number > 14
  3. Carousel starts on various indexes which are not the one you passed in as a prop.

Other Notes

I commented with this snack on #496, but I don't think it was noticed, and is likely to be a different issue now that I read that issue more closely, so I figured I'd open a new one. Thanks for your help!

GH974 commented 5 years ago

@thekevinbrown Hi, I have the same issue, i resolve this in use initialNumToRender={data.length}, but i slow performance

thekevinbrown commented 5 years ago

That’s a workaround we could definitely try @gh974, but I’m hoping that as flatlist is a virtual list that we don’t have to resort to rendering everything ever, or it’ll cause some really terrible performance in our use case. There are sometimes a lot of items.

creativemind1 commented 5 years ago

Today, I faced the same issue. The first time doesn't work properly except less than 15 index number. I have had a long list of items and I set to lets say 50th item and it showed me 10th index number

rontalx commented 5 years ago

Experiencing the same issue exactly. managed to solve either by useScrollView / initialNumToRender={data.length} but obviously in large lists this would be problematic in terms of performance.

btw, if I monkey patch the library code and delete the getItemLayout and initialScrollIndex overrides here: https://github.com/archriss/react-native-snap-carousel/blob/master/src/carousel/Carousel.js#L1301 then I can use these methods to tell FlatList to start loading items from the given index, and it solves the issue - not sure why they where overridden.

It seems that the library doesn't know to re-snap onto items in the next batches after the initialNumToRender by FlatList.

creativemind1 commented 5 years ago

I have like 1000 items to be displayed under this carousel and I have to set the initialScrollIndex to 900th, but unfortunately due to memory issue, my App is getting closed. Do you have any suggestion for this?

bd-arc commented 5 years ago

I strongly advise against initialNumToRender={data.length} since you will encounter critical performance issues.

The reasons why getItemLayout and initialScrollIndex were overridden in the first place have been explained in #193 and boil down to the fact that FlatList was (and still is) very buggy.

To be honest, I'd be pretty happy to finally get rid of the override!

Before that, can each one of you try @rtalwork's PR #547 and tell me if they experience any side effect?

rontalx commented 5 years ago

@thekevinbrown @creativemind1 Can you check if using getItemLayout & initialScrollIndex after removing the override resolves it for your use-case?

GH974 commented 5 years ago

Hi, i have tested but it not resolve this.

thekevinbrown commented 5 years ago

Hey @rtalwork, I tried your branch both in my app and with a local copy of the snack I posted at the top of the issue and saw the same behaviour as before.

Have you definitely tested with the snack I provided at the top of the issue and were able to resolve it with the PR?

rontalx commented 5 years ago

@thekevinbrown Did you pass to the Carousel component the relevant getItemLayout & initialScrollIndex properties for your use-case? for example:

          firstItem={this.props.firstSlideIndex}
          initialScrollIndex={this.props.firstSlideIndex}
          getItemLayout={(_: ListItemData, index: number) => ({
            length: CARD_WIDTH,
            offset: CARD_WIDTH * index,
            index,
          })}

The branch just enables usage of them, so using the branch without implementing the props wouldn't make any change. I haven't tested your snack specifically but I've reproduced exactly the same issue and now it's resolved also in my production environment on bigger lists. LMK how it goes

GH974 commented 5 years ago

Hi, i confirm is better at 60%, but not 100% perform but i have a complexe component inside carrousel. to better performance in my renderModalCarrousel i have added this.

renderModalCarrousel({item, index}){ if( index==this.state.activeIndex || index==this.state.activeIndex+1 || index==this.state.activeIndex-1 ){ return (<MyComplexComponent />); } else { return null; } }

and my new carrousel


<Carousel
 ref={(c) => this._carouselActu = c}
 data={this.state.contentActu}
 renderItem={this.renderModalCarrousel.bind(this)}
 firstItem={this.state.activeIndex}
 initialScrollIndex={this.state.activeIndex}
 getItemLayout={(data, index) => (
     {length: width, offset: width * index, index}
 )}
 initialNumToRender={1}
 maxToRenderPerBatch={1}
 sliderWidth={width}
 itemWidth={width}
 removeClippedSubviews={true}               
 onBeforeSnapToItem={(activeIndex) => {
     this.setState({activeIndex:activeIndex, showWebSite:false, isLoadWebView:false, indexArticleOpen:0});
 }}
/>`
SergeyPanchenkoWeWork commented 5 years ago

@GH974 I would suggest not passing this.state.activeIndex to firstItem and initialScrollIndex and not update it dynamically (set it to value that won't be changed), it caused some issue when we did the same.

This snippet from componentWillReceiveProps in Carousel.js shows how firstItem changes might effect

else if (nextFirstItem !== this._previousFirstItem && nextFirstItem !== this._activeItem) {
            this._activeItem = nextFirstItem;
            this._previousFirstItem = nextFirstItem;
            this._snapToItem(nextFirstItem, false, true, false, false);
        }
thekevinbrown commented 5 years ago

@rtalwork I'll have a go when I get a chance.

At this stage I'm just looking to solve my problem, but I would say in regards to the longer term here if firstItem can't work as stated in the docs (e.g. you just put an index in there and it works), it'd be best for it to be removed and to be replaced with a section in the docs explaining how to do this with the initialScrollIndex and getItemLayout props in my opinion.

rontalx commented 5 years ago

@thekevinbrown I think that it is better that implementation details such as usage of initialScrollIndex & getItemLayout are not exposed by the carousel, meaning that still firstItem should be used, but the carousel would implement the relevant initialScrollIndex & getItemLayout logic if the required props are provided (e.g. firstItem, itemWidth). I'l try to open a PR for that hopefully soon.

thekevinbrown commented 5 years ago

Hey @rtalwork,

I'm trying to replicate what you're doing to solve this problem, and I keep getting:

E/ReactNativeJS: TypeError: TypeError: undefined is not an object (evaluating 'this.props.itemWidth')

    This error is located at:
        in VirtualizedList (at FlatList.js:632)
        in FlatList (at createAnimatedComponent.js:151)
        in AnimatedComponent (at Carousel.js:1361)
        in Carousel (at component.tsx:375)
        in RCTView (at View.js:45)
        in View (at SafeAreaView.js:37)
        ...etc...

Is there anything else you did to get this to work? I've tried creating a PR for the carousel itself and get the same issue there too.

thekevinbrown commented 5 years ago

I got it working in my app by replicating what @GH974 did. Will leave this open until there's a PR to fix it in the core carousel.

Foskas commented 5 years ago

@thekevinbrown any updates on this ?

thekevinbrown commented 5 years ago

I tried to do a PR for the carousel and couldn’t reliably get access to its props for passing down to FlatList for some reason. I worked around it as above.

Foskas commented 5 years ago

@thekevinbrown can you provide a code sample or snack or something for me to get based on ? Thanks :)

thekevinbrown commented 5 years ago

Sure: https://github.com/archriss/react-native-snap-carousel/issues/538#issuecomment-507552623

kganser commented 5 years ago

I have found a workaround for this issue (applied to bug report demo for horizontal carousel):

contentContainerCustomStyle={{
  minWidth: windowWidth * data.length
}}

This allows the initial scrollTo to get to where the initial slide should be in the scrollview content container by making it wide enough initially.

Marishwaran99 commented 5 years ago

Hello, I had used snapToItem method in onLayout callback FIRST_ITEM = "SOME_VALUE" <NavigationEvents onDidFocus={() => { const { idx} = this.props.data; this.setState({ sliderActiveIndex: idx }); FIRST_ITEM = idx; this._slider1Ref.snapToItem(FIRST_ITEM); }} /> ... <Carousel ref={c => (this._slider1Ref = c)} data={this.state.images} renderItem={this._renderItem} sliderWidth={wp("100%") - 8} itemWidth={wp("100%") - 16} activeSlideAlignment={"start"} inactiveSlideScale={1} layout="default" onLayout={() => { console.log("hello"); this._slider1Ref.snapToItem(FIRST_ITEM); }} />

Foskas commented 5 years ago

Is there any PR to fix this ?

bd-arc commented 5 years ago

Just got rid of the getItemLayout and initialScrollIndex override in version 3.8.3. Using these props should help you all get rid of the issue.

Keep me posted!

mrarronz commented 4 years ago

I have found a workaround for this issue (applied to bug report demo for horizontal carousel):

contentContainerCustomStyle={{
  minWidth: windowWidth * data.length
}}

This allows the initial scrollTo to get to where the initial slide should be in the scrollview content container by making it wide enough initially.

Hello @kganser I've tried your solution, it does work. But I got a new problem, the active card is not aligned center with changing width enough for contentContainerCustomStyle. The active card should be expected to align at center, but it aligns at left. Have you encountered that problem? I've tried a lot of styles, but didn't resolve my issue. Any suggestion will be appreciated!

Here's my code to use Carousel, sliderWidth equals to the window width

<Carousel
        data={dataSource}
        renderItem={this.renderItem}
        sliderWidth={sliderWidth}
        itemWidth={itemWidth}
        firstItem={activeIndex}
        inactiveSlideScale={0.95}
        inactiveSlideOpacity={1}
        removeClippedSubviews={false}
        contentContainerCustomStyle={{
          minWidth: sliderWidth * dataSource.length,
        }}
      />
mrarronz commented 4 years ago

@kganser I have 30 cards to display at horizontal, I found that when swiping the cards to 15th, the card finally displayed at center, after that I can swipe left and right, each active card showed on center of the screen, but it aligns at left when the active index set to more than 15 after relaunching app. Hope I have explained my problem clearly, thanks.

SSTPIERRE2 commented 4 years ago
contentContainerCustomStyle={{
  minWidth: windowWidth * data.length
}}

This allows the initial scrollTo to get to where the initial slide should be in the scrollview content container by making it wide enough initially.

This fixed the issue for me, but for added context my items are full screen pictures, so the centering issue doesn't apply to me, luckily! 😄

roybselim commented 4 years ago

I have found a workaround for this issue (applied to bug report demo for horizontal carousel):

contentContainerCustomStyle={{
  minWidth: windowWidth * data.length
}}

This allows the initial scrollTo to get to where the initial slide should be in the scrollview content container by making it wide enough initially.

I tried this on a horizontal carousel and it does work

PitzTech commented 3 years ago

I have found a workaround for this issue (applied to bug report demo for horizontal carousel):

contentContainerCustomStyle={{
  minWidth: windowWidth * data.length
}}

This allows the initial scrollTo to get to where the initial slide should be in the scrollview content container by making it wide enough initially.

Hello @kganser I've tried your solution, it does work. But I got a new problem, the active card is not aligned center with changing width enough for contentContainerCustomStyle. The active card should be expected to align at center, but it aligns at left. Have you encountered that problem? I've tried a lot of styles, but didn't resolve my issue. Any suggestion will be appreciated!

Here's my code to use Carousel, sliderWidth equals to the window width

<Carousel
        data={dataSource}
        renderItem={this.renderItem}
        sliderWidth={sliderWidth}
        itemWidth={itemWidth}
        firstItem={activeIndex}
        inactiveSlideScale={0.95}
        inactiveSlideOpacity={1}
        removeClippedSubviews={false}
        contentContainerCustomStyle={{
          minWidth: sliderWidth * dataSource.length,
        }}
      />

contentContainerCustomStyle={{ minWidth: sliderWidth * dataSource.length, }}

solved the problem with firstItem, on RN 0.64.5 and react-native-snap-carousel 3.8.4

Thank You!

dohooo commented 3 years ago

Sorry, please allow me to advertise for my open source library! ~ I think this library react-native-reanimated-carousel will solve your problem. It is a high performance and very simple component, complete with React-Native reanimated 2

akbari6776 commented 2 years ago

I used firstitem, getItemLayout, initialNumToRender, and initialScrollIndex many times, but it didn't work. I finally used loopClonesPerSide and set its value to data.length and started displaying from first item.

loopClonesPerSide= { data.length }

Malitowska commented 2 years ago

thank you @Marishwaran99 although I can't use your solution without animation I mean that

this._slider1Ref.snapToItem(FIRST_ITEM, false); should disable the animation but it breaks the functionality itself and puts carousel to first item like nothing changed :(

with animation it works.. so it's like almost

umair-khanzada commented 4 months ago

contentContainerCustomStyle={{ minWidth: windowWidth * data.length }}

It worked for me, I also got lucky to have a full-view slider.

For the horizontal slider, you need to use minHeight instead of minWidth contentContainerCustomStyle={{minHeight: height * data.length}}