react-native-snap-carousel icon indicating copy to clipboard operation
react-native-snap-carousel copied to clipboard

firstItem doesn't work reliably

Open thekevinbrown opened this issue 5 years ago • 32 comments

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!

thekevinbrown avatar Jun 14 '19 07:06 thekevinbrown

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

GH974 avatar Jun 14 '19 09:06 GH974

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.

thekevinbrown avatar Jun 14 '19 12:06 thekevinbrown

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

creativemind1 avatar Jun 25 '19 00:06 creativemind1

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.

rontalx avatar Jun 26 '19 13:06 rontalx

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?

creativemind1 avatar Jun 26 '19 13:06 creativemind1

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?

bd-arc avatar Jun 26 '19 16:06 bd-arc

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

rontalx avatar Jun 30 '19 07:06 rontalx

Hi, i have tested but it not resolve this.

GH974 avatar Jun 30 '19 15:06 GH974

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?

thekevinbrown avatar Jul 01 '19 07:07 thekevinbrown

@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

rontalx avatar Jul 02 '19 06:07 rontalx

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});
 }}
/>`

GH974 avatar Jul 02 '19 07:07 GH974

@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);
        }

SergeyPanchenkoWeWork avatar Jul 02 '19 07:07 SergeyPanchenkoWeWork

@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.

thekevinbrown avatar Jul 04 '19 08:07 thekevinbrown

@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.

rontalx avatar Jul 07 '19 13:07 rontalx

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 avatar Aug 30 '19 05:08 thekevinbrown

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.

thekevinbrown avatar Sep 11 '19 00:09 thekevinbrown

@thekevinbrown any updates on this ?

Foskas avatar Sep 18 '19 15:09 Foskas

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.

thekevinbrown avatar Sep 18 '19 16:09 thekevinbrown

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

Foskas avatar Sep 18 '19 18:09 Foskas

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

thekevinbrown avatar Sep 19 '19 04:09 thekevinbrown

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.

kganser avatar Sep 20 '19 16:09 kganser

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); }} />

Marishwaran99 avatar Sep 21 '19 11:09 Marishwaran99

Is there any PR to fix this ?

Foskas avatar Oct 02 '19 09:10 Foskas

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!

bd-arc avatar Oct 27 '19 10:10 bd-arc

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 avatar Feb 18 '20 07:02 mrarronz

@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.

mrarronz avatar Feb 18 '20 07:02 mrarronz

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! 😄

SSTPIERRE2 avatar Feb 24 '20 15:02 SSTPIERRE2

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

roybselim avatar Nov 11 '20 08:11 roybselim

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!

PitzTech avatar Jul 26 '21 13:07 PitzTech

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

dohooo avatar Oct 08 '21 05:10 dohooo