Problem on re-rendering of component, setting a new firstItem index

[KevKo1990]

I have several miniComponents listed in my carousel and a user can select and deselect these miniComponents on different screens to be visible in the carousel or not.
Removing and adding them works splendidly (the miniComponents.map function below).

So an actual rerendering is on this page with the Carousel happening.

But if remove one of the miniComponents, my activeMiniCompIndex might chance as components now move to different positions. I set in firstItem the activeMiniCompIndex, the Carousel stays however on the same position.
I checked via console.log output that the activeMiniCompIndex is changing and that a rerender is executed in the component containing the carousel, however the carousel is not.

<Carousel
            ref={carousel => {
              this._carousel = carousel;
            }}
            firstItem={activeMiniCompIndex}
            sliderWidth={Dimensions.get('window').width}
            itemWidth={Dimensions.get('window').width * 0.75}
            scrollEndDragDebounceValue={0}
            inactiveSlideOpacity={0.8}
            contentContainerCustomStyle={S.carouselContainer}
            onSnapToItem={index => {
              this.activeMiniCompKeyStorage.set(miniComponents[index].key);
              if (index !== activeMiniCompIndex) {
                tracker.trackEvent('miniComponents', 'scrollTo', { label: miniComponents[index].name });
                this.setState({ activeMiniCompKey: miniComponents[index].key });
              }
            }}
          >
            {miniComponents.map((miniComponent, index) =>
              <View key={index}>
                {this.renderContent(miniComponent)}
              </View>
            )}
</Carousel>

I checked your documentation and past issues, unfortunately I couldn't find a solution to this problem.

Thank you in advance for your help!

[bd-arc]

Hi @KevKo1990,

Just a quick thought, but does it work if you add extraData={this.state} to the <Carousel />? (from FlatList's doc)

And if it doesn't, can you set up a simple test case so I can easily reproduce the issue?

[KevKo1990]

@bd-arc
Thanks, it seemed first to work but after some tests, it does not...

I will try to make an easy executable example of my problem!

[KevKo1990]

I had a look into your Carousel componentWillReceiveProps() function and I think I found the problematic part:
else if (nextFirstItem !== previousFirstItem && nextFirstItem !== activeItem)
https://github.com/archriss/react-native-snap-carousel/blob/master/src/carousel/Carousel.js#L214

I checked in your code the _onScroll function. It is just changing the activeItem, however not the firstItem. https://github.com/archriss/react-native-snap-carousel/blob/master/src/carousel/Carousel.js#L402-L457

In the following case I will receive a not wished behavior with being stuck on an empty page:

1. open the carousel with firstItem = 0 and activeItem=0
2. Leave carousel page, adding and removing components in different screen
3. Go back to carousel, all changes of components worked
4. Swipe to a different compenent (changing e.g. activeItem=4 via SnapToItem / _OnScroll)
5. Leaving carousel, and remove one random, not the selected compenent (especially in this case not 4)
6. Going back to the carousel, it perfectly changed its position to the selected component as via componentWillReceiveProps a new firstItem was given to the Carousel and activeItem !== newFirstItem and previousFirstItem !== newFirstItem.
7. Leaving carousel, and remove the selected compenent (especially in this case 4)
8. Going back to the carousel, see that carousel is on an empty/wrong position but component at index 0 is active (firstItem stayed in this usage behavior on the position of component 4 and with that the above mentioned else if condition is not fulfilled and in the componentWillReceiveProps function is no snapToItem function called)

I thought about several solutions, however none of them did work for me:

• Defining in onSnapToItem= a new firstItem would not work as the following would be always the case: activeItem==firstItem
• Adding an additional else if condition that would be just executed if my above condition is valid and never else (closest one was to compare previous maxIndex with new maxIndex, but that does become true even I removed other components...)

[bd-arc]

Yep, that's a real head-scratching issue. A while ago, I anticipated this kind of problem and tried to figure out a solution, but didn't come up with anything satisfying (and ended up forgetting it...). Now is the time to tackle it for good!

First, after 7 (when you remove the currently selected component), can you confirm that this condition is met?

Then, would something along those lines work?

let nextActiveItem = activeItem || activeItem === 0 ? activeItem : nextFirstItem;

if (nextActiveItem > itemsLength) {
    nextActiveItem = itemsLength - 1;
}

Finally, and I don't mean to insist, but a test case would made things easier for me :-)

[KevKo1990]

https://github.com/KevKo1990/Snap-Carousel-Fix-Dynamic

I created a example repo with not completely our problem but a really close one (if I deselect another component than the selected one, it is in this repo now as well problematic). If this would be fixed, our problem would be as well. ;)

Just to show you the problem that I rebuild:

Does this help you?

(btw thank you for your example, I just used it, hope you do not mind... ;))

[KevKo1990]

Concerning your proposed fix, it would not solve our problem.

I can confirm, the condition is met as soon as we remove or add a component.
However, if we remove the actual selected component, we want to jump to a defined position by ourselves (in our case the first one).

[bd-arc]

Hey @KevKo1990,

Thanks for the awesome example! This is really going to help me figure out a solution. I'll take a look at it today and keep you posted ;-)

By the way, I'm curious about how you created your gif file. I'm always hitting GitHub's 5 Mo limit with mines, with as little as a 4 to 5 seconds duration...

[KevKo1990]

@bd-arc

No worries, I asked for your help ;)

I am developing right now on Ubuntu and there I found this tool. I think it is really nice!
https://github.com/phw/peek/blob/master/README.md

[bd-arc]

@KevKo1990 Thanks for sharing your tool! It won't work for me, but it's still good to know.

Anyway, I finally started working on the issue and, well, the first thing to be noted is that Android and iOS do not behave the same at all: http://www.giphy.com/gifs/l1J9Geez5TJ78POuI

Usually all hell breaks loose 2 to 3 minutes after such a revelation...

[bd-arc]

@KevKo1990 By the way, it seems that your example is missing an Android version ;-)

[bd-arc]

Hey @KevKo1990,

So, fixing the main issue (scrolling back to the previous slide on Android) was no big trouble. But now comes another one: displaying the active animation for the previous slide.

I might implement a recent PR idea to see if that helps.

[bd-arc]

Good news: this commit should solve the issue. It wasn't that easy, but I think I got it :-)

Can you try the fix and let me know if it works for you and if you don't find any drawbacks?

[KevKo1990]

@bd-arc Thank you! Sorry, I am right now a bit busy and saw your replies just now.
I don't understand the point with no android version as I checked just for android, however I am glad that you figured it out.

I will try to check tonight (JST) and let you know if the fix works!

[bd-arc]

I just meant that your repo didn't contain the android source code. But adding it wasn't too much trouble ;-)

Make sure to test the latest commit on master, because I made a substantial update and then modified the fix accordingly.

[KevKo1990]

Thank you!

Hm, I am facing now a problem. In my provided repo, I solved the carousel entries via the data props, but in my actual problem case, I have the above structure:

<Carousel
          ...
          >
            {miniComponents.map((miniComponent, index) =>
              <View key={index}>
                {this.renderContent(miniComponent)}
              </View>
            )}
</Carousel>

I replaced now my existing above solution with data and renderItem which are now required...
Then it works, it LGTM!

I think you want to fix above mentioned problem or is this a mistake from my side?
Furthermore, I now recognize that Pagination has at least in my current implementation some problems... (as just the last Pagination point is visible...). Can this be based on this change?

PS: Sorry for not uploading the android code... seems to be that I made a mistake with the .gitignore file...

[bd-arc]

@KevKo1990 I'm not sure that I totally get what you wrote.

What I can tell you is that I've tried the new code with your custom example and everything is working perfectly, both slider and pagination.

Here is a quick screencast:

[KevKo1990]

Forget my comment from above, a colleague of mine implemented the Carousel view, maybe it was still an old implementation... (we had this solution still working https://github.com/archriss/react-native-snap-carousel#migration-from-version-2x).

I have to update my code from my app. I think it is fine, I will tell you asap if not.
For now, it looks good to me!

PS: I am quiet busy these days, so, it may take some time until I give you a final confirmation, sorry for that!

[KevKo1990]

I am facing one problem after some more checks.

If set in my repo in src/static/entries.js showInitial: true for the second entry and in src/components/TopSlider.js firstItem=1 and activeDotIndex=1 and then start the app, then I receive the following:

Actually, my firstItem set value should be active (, so the visible carousel component), not the one at position 0. Pagination works correct, the carousel does not.

[bd-arc]

@KevKo1990 Does it work if you use the code from this commit?

[KevKo1990]

Yes, I can confirm it is fine. I tested it on my example repo and my app.

I tested further my app and I recognized now if I rerender my carousel with a reduced set of components because I deselected some of them, that onSnapOnItem is called even it is not based on user interaction. This is problematic for me, if I define especially to go back to the 0 position when I removed my actual visible component. Then instead it stops on a position before, e.g. 1, because onSnapOnItem gets called and it saves this index as actual index...

I think it is not intended to call onSnapOnItem while just rerendering the Carousel?

[bd-arc]

@KevKo1990 Im glad you're testing the plugin thoroughly :-) I am triggering onSnapOnItem in order to be able to automatically update the <Pagination /> component, but I agree that this is a bad idea. I'll change that, but you'll then have to update pagination manually.

Be aware that the commit I made you test is an older one; it was a previous attempt at solving your issue, but I've overridden it because I wasn't happy with using setState() in componentDidUpdate(). You'll have to check if this is still an issue.

I'll keep you posted as soon as I've updated the code.

[KevKo1990]

Thank you!

@bd-arc I really love your solution, so it is worth!
I think you did a great job here.

I will probably test it then altogether, take as much time as you need. ;)

[bd-arc]

@KevKo1990 I'll let you play with this commit ;-)

[KevKo1990]

Hm...

So actually the onSnapItem is not longer called, but I have now a different problem :/

I rerender my Carousel with a firstItem=0 after removing my actual selected component... But it goes to into index 1...

[bd-arc]

Ok, here is a new commit for you to try. This one should address the active state being displayed on the wrong item when using a firstItem other than 0.

I if doesn't solve your issue, could you update your example repo with a reproductible test case?

[KevKo1990]

I downloaded your commitment and extracted it into the node_moduls/react-native-snap-carousel folder and replaced the existing version (this is how I test it, right? Just want to confirm)

I still have a problem with it - as you can see, as soon as I remove my favorite component, I want to have it jump back to 0. Pagination works, the carousel itself not...

[bd-arc]

Hey @KevKo1990,

You can either copy/paste the raw code, like you did, or reference the commit in your package.json file:

"react-native-snap-carousel": "https://github.com/archriss/react-native-snap-carousel#f38ef07"

and then run npm install or yarn depending on what you're using.

Regarding the issue, what you're showing is the expected behavior: if you remove an item, the closest one becomes active. And, as I've previously mentioned, since the callback is not fired the Pagination component won't automatically update and will fall back to its default value (0).

I think that it's a little dangerous to always go back to the first item if the current one is deleted. Imagine a list of hundreds of items; it would result in a pretty bad user experience.

What would be the best behavior in your opinion?

[KevKo1990]

I talked to my team, we are fine with your solution. Your described behavior should resolve our issue, we are totally fine with it. 👍

[bd-arc]

Ok, perfect! Then you can start using version 3.2.1 right away ;-)