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.36k stars 2.29k forks source link

[Android] lockScrollWhileSnapping={true} (Version 3.3.3) #176

Closed kadoshms closed 7 years ago

kadoshms commented 7 years ago

Hi! First of all I'd like to thank you for this great library, it saved me some precious time. Iv'e started using it a while ago and still watching changes, mostly Android performance improvements since my usage of this component is a bit non-trivial

Well, I just noticed that you added the lockScrollWhileSnapping prop - and got excited about it, and rushed to use it. Now I get this exception when scrolling:

whatsapp image 2017-10-09 at 11 26 57

My carousel renderer:

renderCarousel() {
        const windowHeight = Dimensions.get('window').height;
        const { onSwipe } = this.props;

        return (
            <Carousel renderItem={this.renderItem.bind(this)}
                      data={this.props.children}
                      scrollEndDragDebounceValue={0}
                      activeSlideOffset={0}
                      vertical={true}
                      scrollEventThrottle={100}
                      apparitionDelay={10}
                      useNativeOnScroll={true}
                      sliderHeight={windowHeight}
                      itemHeight={windowHeight}
                      lockScrollWhileSnapping={true}
                      inactiveSlideOpacity={1}
                      inactiveSlideScale={1}
                      onScroll={onSwipe}
            />
        );
    }

I am looking forward for your opinion! Thanks.

bd-arc commented 7 years ago

Hi @kadoshms,

If you've been watching the repo for a while, I guess you've already read this important note regarding Android. If not, make sure to do so ;-)

Regarding your issue, I'm quite surprised because it means that, sometimes, setNativeProps() is undefined. I have no idea about why this can happen, probably something cryptic when mounting/unmounting Animated components; it wouldn't be the first odd issue with these. At least we should be able to guard against it.

Could you try to modify lines 362-363 of file Carousel.js to the following and tell me if this solves the problem? Can you also tell me if you see the logs in your console?

if (this._flatlist && this._flatlist.setNativeProps) {
    console.log('SNAP CAROUSEL | Set `scrollEnabled` value', value);
    this._flatlist.setNativeProps({ scrollEnabled: value });
    this._scrollEnabled = value;
}

By the way, prop useNativeOnScroll has been removed in version 3.2.0 because scroll animations will now always be native-powered (it was required with the revamp of the callback mechanism). And, as a result, scrollEventThrottle value will always be set to 1 regardless of your custom value.

kadoshms commented 7 years ago

@bd-arc thanks for your quick reply! This indeed solve the problem. Do you have plans for releasing this fix anytime soon?

bd-arc commented 7 years ago

@kadoshms I'll make sure to release it today ;-)

Just to make sure that there wasn't a deeper issue at stake, can you confirm that the log is showing up in your debug console?

kadoshms commented 7 years ago

@bd-arc sorry for forgetting about it, unfortunately it does not appear in my console.

bd-arc commented 7 years ago

@kadoshms Damn! This means that the prop will do nothing for you. I don't understand it since I've tested it on a bunch of devices without having any issue.

Which version of React Native are you currently using? Also, can you try the provided example and tell me if it works when patching it?

kadoshms commented 7 years ago

I am using React native 0.46.1 (comes along with Expo sdk 19.0.0 which I am using and not able to upgrade at the moment). Should I try to upgrade my react native version?

bd-arc commented 7 years ago

I'm not aware of any specific issue linked to RN 0.46.x. But in order to understand the matter at stake, I would really appreciate it if you could provide me with a feedback regarding how the example works for you since it's using RN 0.48.4.

There is also something else I am thinking about: do you, by any chance, use stateless components within the carousel?

Apparently, one should avoid using stateless components when they are rendered inside scroll components (see this React Native thread). They should be migrated to regular components that extend Component or PureComponent (see this comment or this one).

kadoshms commented 7 years ago

@bd-arc I will upgrade and give you feedback soon! (it will take a bit but I will, this feature is a game changer for me and I would really want to use it).

BTW - I am using PureComponents :)

bd-arc commented 7 years ago

Good to know!

Note that if you don't want to upgrade your project (which, I know for a fact, can get ugly real quick), you just have to:

This could actually be way faster for you ;-)

bd-arc commented 7 years ago

Hi @kadoshms,

Any news regarding the issue?

kadoshms commented 7 years ago

I wasnt able to check yet, but I am deffinelty want to do that soon! please leave this ticket open :)

bd-arc commented 7 years ago

Don't worry, I'm not closing it until we've got to the bottom of the issue ;-)

kadoshms commented 7 years ago

@bd-arc in the meantime, can you please publish the fixed you suggested above with a fix tag?

bd-arc commented 7 years ago

@kadoshms That's the plan. But there is another issue I want to fix before publishing a new release.

I'll keep you posted!

bd-arc commented 7 years ago

@kadoshms The fix has been published in version 3.3.4 ;-)

kadoshms commented 7 years ago

Your'e awesome, thanks!

bd-arc commented 7 years ago

@kadoshms No problem ;-)

I noticed that you've closed the issue; does it mean that you've been able to use lockScrollWhileSnapping with success?

kadoshms commented 7 years ago

yes!