Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
kadoshms opened this issue Oct 9, 2017 · 18 comments
Closed

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

kadoshms opened this issue Oct 9, 2017 · 18 comments

Comments

@kadoshms
Copy link

kadoshms commented Oct 9, 2017

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
Copy link
Contributor

bd-arc commented Oct 9, 2017

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
Copy link
Author

kadoshms commented Oct 9, 2017

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

@bd-arc
Copy link
Contributor

bd-arc commented Oct 9, 2017

@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
Copy link
Author

kadoshms commented Oct 9, 2017

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

@bd-arc
Copy link
Contributor

bd-arc commented Oct 9, 2017

@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
Copy link
Author

kadoshms commented Oct 9, 2017

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
Copy link
Contributor

bd-arc commented Oct 9, 2017

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
Copy link
Author

kadoshms commented Oct 9, 2017

@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
Copy link
Contributor

bd-arc commented Oct 9, 2017

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
Copy link
Contributor

bd-arc commented Oct 13, 2017

Hi @kadoshms,

Any news regarding the issue?

@kadoshms
Copy link
Author

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

@bd-arc
Copy link
Contributor

bd-arc commented Oct 13, 2017

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

@kadoshms
Copy link
Author

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

@bd-arc
Copy link
Contributor

bd-arc commented Oct 15, 2017

@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
Copy link
Contributor

bd-arc commented Oct 16, 2017

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

@kadoshms
Copy link
Author

Your'e awesome, thanks!

@bd-arc
Copy link
Contributor

bd-arc commented Oct 16, 2017

@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
Copy link
Author

yes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants