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

🔥 4.0.0 beta.x #678

Open
wants to merge 35 commits into
base: master
Choose a base branch
from
Open

🔥 4.0.0 beta.x #678

wants to merge 35 commits into from

Conversation

bd-arc
Copy link
Contributor

@bd-arc bd-arc commented Apr 6, 2020

Release notes

Everything you want to know about the upcoming version is here

Go on and comment below, ask your questions, provide feedback, and share your insights 😉

@AAAstorga
Copy link

Awesome work. I am looking forward to this. One thing I noticed when testing 4.0.0-beta.2 is the ability to only move one item at a time is gone. I tested on iOS with both useExperimentalSnap on and off. This is one of the big reasons I use this library. I need the ability to only page one item at a time no matter how much momentum was used. Is this intentional?

@bd-arc
Copy link
Contributor Author

bd-arc commented Apr 7, 2020

@AAAstorga Well, the logic is completely different and we're now leaving this to the native scroll instead of trying to compensate for it with JS hacks. That's definitely a tradeoff.

However, I suggest you take a closer look at useExperimentalSnap's description. You can get what you're after on top of the smooth native scrolling if you set both useExperimentalSnap and disableIntervalMomentum to true ;-)

paddingRight: this._getContainerInnerMargin(true)
},
contentContainerCustomStyle || {}
contentContainerCustomStyle || {},

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this come before the next line? I would like the ability to add extra padding if I have a Header / TabBar. The contentContainerCustomStyle padding gets overwritten if I choose not to use useExperimentalSnap

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I totally understand the need, but the issue is that it will then mess with the inner logic (the active item's calculation will be completely off). However, now that I think about it, with the updated _getActiveItem() logic it might just work out-of-the-box.

Do you mind doing a few tests on your end? If you move this line after the override line 1080 and change the padding, is onSnapToItem still firing reliably and with the proper index? With all 3 different activeSlideAlignment values?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AAAstorga Did you ever have a chance to test what we mentioned above? :-)

@r0b0t3d
Copy link
Contributor

r0b0t3d commented Apr 10, 2020

I am using rn 0.62.2 and got this warning

%s: Calling `getNode()` on the ref of an Animated component is no longer necessary. You can now directly use the ref instead. This method will be removed in a future release. FlatList

@bd-arc
Copy link
Contributor Author

bd-arc commented Apr 20, 2020

@r0b0t3d This is the issue described in #673. Unfortunately this is is a big one.

Read this comment to understand why.

@ajp8164
Copy link

ajp8164 commented Apr 24, 2020

I have been researching the issue of smooth scrolling and came across this exciting new update.. This carousel is the center piece of our app and we really want to have an excellent experience - well, this update brings it! I installed beta 2, removed the deprecated properties and wow - fantastic. The feel and usability is excellent - thank you to all for this great improvement.

Since our use case is pretty vanilla; simple carousel for swiping cards, we've decided to upgrade the app now and keep a watch on issues. Thanks again for all your hard work!

Update - The pagination dot doesn't update until the slide completely stops. Would be an improvement to have the dot update as soon as the destination slide is determined.

Update 2 - I see prop onScrollIndexChanged and replaced onSnapToItem with it. The carousel i'm using for test has just three cards. When swiping from card 0 to 1, 1 to 2, or 2 to 1 the slide momentum stops early and the slide really snaps into place. However, when swiping from slide 1 to 0 the momentum of slide 0 is smooth until stop (no snap in place). Pagination works as expected though ;-)

          <Carousel
            ref={c => {
              this.state._carousel = c;
            }}
            firstItem={this.state.activeSlide}
            data={visibleCards}
            renderItem={this._renderCard}
            sliderWidth={sliderWidth}
            itemWidth={itemWidth}
            inactiveSlideScale={0.9}
            inactiveSlideOpacity={1}
            activeSlideAlignment={'center'}
            onScrollIndexChanged={index =>
              this.setState({activeSlide: index, swiped: true})
            }
          />

Update 3 - I noticed that loop={true} doesn't loop. I tested with 8 cards in the carousel and the rendered set is 14 cards and stops at both ends. So 6 of the 8 cards are duplicates.

@bd-arc
Copy link
Contributor Author

bd-arc commented Apr 24, 2020

@ajp8164 Thanks a lot for the feedback! We're already using it in production as well — couldn't wait to please our users with that smooth scrolling :-)

Re: Pagination

I'll admit I haven't tried it yet. My guess is that we need to connect the relevant logic to onScrollIndexChanged for the dots to update while scrolling.

Re: onScrollIndexChanged

To be honest, I don't expect it to work properly given your example. Basically, you're updating the state in the callback, but as a result the firstItem prop is updated too and this resets the carousel. Is it working properly if you remove the firstItem prop?

Re: Loop

This is surprising! I've made sure to test the loop thoroughly and can confirm it was working on both iOS and Android. Do you mind sharing your setup?

@ajp8164
Copy link

ajp8164 commented Apr 24, 2020

Great call on the firstItem value - removing it fixed the slide snap problem. It also solves the pagination dot rendering - it no renders while the slide is slowing down.

Regarding loop - After some experimentation I have it working. The slides in the loop are vertical stack. sliderHeight and itemHeight are set. sliderWidth and itemWidth are not. When i add itemWidth the looping works correctly. Setting sliderWidth didn't have any effect.

Your reply helped greatly - thank you very much, I appreciate you guys!

@jacquesdev
Copy link

About to take the plunge by implementing beta (first time using this plugin). Just wanted to get an idea of how close we are to beta 3?

@alessandro-bottamedi
Copy link

Hi, i've tested version 4 beta 2, it works fine, the only problem I noticed is that the snapToItem method does not work on the first element with index 0, eg:
CarouselRef.current.snapToItem(0, false, false); don't works, CarouselRef.current.snapToItem(1, false, false); works fine.
I try to investigate the code to find out why.

@bd-arc
Copy link
Contributor Author

bd-arc commented May 26, 2020

@alessandro-bottamedi Thanks for testing! It's actually been fixed with #696.

@bd-arc bd-arc mentioned this pull request May 26, 2020
9 tasks
@bd-arc
Copy link
Contributor Author

bd-arc commented May 26, 2020

@r0b0t3d Just implemented the solution you suggested to get rid of the getNode() warning. Thanks for that!

However I didn't test it on RN < 0.60...

Version 4.0.0-beta.3 is available to everyone who wants to give it a try!

@alessandro-bottamedi
Copy link

IMG_3403
with beta 3 i get this error when try to snapToItem (android and iOs, RN 0.61.5)

@bd-arc
Copy link
Contributor Author

bd-arc commented May 26, 2020

@alessandro-bottamedi Is it working if you remove the following in your local node_modules folder?

If yes, then the deprecation fix is unfortunately doing more harm than good...

There are other ideas mentioned in #673, but none of them is perfect.

@alessandro-bottamedi
Copy link

@alessandro-bottamedi Is it working if you remove the following in your local node_modules folder?

If yes, then the deprecation fix is unfortunately doing more harm than good...

There are other ideas mentioned in #673, but none of them is perfect.

Yes, i confirm that everything works correctly without those lines

@r0b0t3d
Copy link
Contributor

r0b0t3d commented May 26, 2020

just upgrade to beta3, works fine on my side. using rn 0.62.2

@r0b0t3d
Copy link
Contributor

r0b0t3d commented May 26, 2020

@alessandro-bottamedi could you change above code to

       if (this._carouselRef && (
            (this._needsScrollView() && this._carouselRef.scrollTo) ||
            (!this._needsScrollView() && this._carouselRef.scrollToOffset)
        )) {
            return this._carouselRef;
        }

This code works in 0.62.2, need your confirm in 0.61.x

@alessandro-bottamedi
Copy link

@alessandro-bottamedi could you change above code to

       if (this._carouselRef && (
            (this._needsScrollView() && this._carouselRef.scrollTo) ||
            (!this._needsScrollView() && this._carouselRef.scrollToOffset)
        )) {
            return this._carouselRef;
        }

This code works in 0.62.2, need your confirm in 0.61.x

It seems that everything works correctly with this change on RN 0.61.5! 👍👍

@r0b0t3d
Copy link
Contributor

r0b0t3d commented May 26, 2020

great! @bd-arc what do you think?

@bd-arc
Copy link
Contributor Author

bd-arc commented May 26, 2020

Great idea @r0b0t3d! And thank you @alessandro-bottamedi for testing it on such a short notice :-)

Want to make a PR for this branch and one for master @r0b0t3d? (Adding a little comment to explain the reason why we had to do that if you don't mind.)

If you're pressed on time, I'll take care of it myself.

@r0b0t3d
Copy link
Contributor

r0b0t3d commented May 26, 2020

it is ok for me. but there is another PR for this case #673. should I create another one?

@bd-arc
Copy link
Contributor Author

bd-arc commented May 26, 2020

@r0b0t3d Sure, go ahead!

Once your two PRs are merged we should finally be able to say goodbye to that issue.

@bd-arc
Copy link
Contributor Author

bd-arc commented May 27, 2020

Ok, version 4.0.0-beta.4 should solve the getNode() issue for everyone.

Kudos to @r0b0t3d 🙌

@nelsonprsousa
Copy link

Hi @bd-arc, thanks for your hard work with this library!

I am trying to test v4. Do you know if there's some away to add the types?

I've tried yarn upgrade -d @types/react-native-snap-carousel@4.0.0-beta.4 but didn't work.

@bd-arc
Copy link
Contributor Author

bd-arc commented May 27, 2020

@nelsonprsousa Well, I'm not the one maintaining those types, so the definitions are not up-to-date with the beta.

The plan would be to migrate the whole library to TypeScript, but I haven't found the time to do so yet...

@nelsonprsousa
Copy link

Oh, my bad 😅

I've already tested 4.0.0-beta.4 and it is working as expected (even parallax images and pagination).

And much faster and smoothly. Awesome job! 🚀

Gonna remove @types/react-native-snap-carousel for now 👍

@kolkol69
Copy link

hey everybody, thanks for the 4.x version, its awesome, just want to let you know that upgrading from 4.0.0 beta.3 to 4.0.0 beta.4 broke horizontal slider fro me, on the second cycle of sliding slide nr 4 appeared to react like it was the last slide(although there were 7 slides), so I could swipe further, only swipe back. The same issue I had with the latest 3.9 version, that's why started to test beta's

@danloiterton
Copy link

ah, that totally makes more sense! 🤦

@27leaves
Copy link

27leaves commented Jan 4, 2021

Why onBeforeSnapToItem was removed? I use it with the old version and onSnapToItem reacts a bit too late. Is there an alternative?

@TuJobs
Copy link

TuJobs commented Jan 8, 2021

In version 4.0.0-beta.6, I have a loop stuck problem on ios, I tried to find the cause in the library and I see now when the stuck list scrollOffSet is not updated causing the problem Trying, checking between nextActiveItem and activeItem is negligible. I skipped the check (nextActiveItem! == this._activeItem) in _onMomentumScrollEnd and everything worked fine. I'm a novice so I can't find out the cause of the bug deeply, someone can and fix it. (react-native 0.63.3)

@reepush
Copy link

reepush commented Jan 9, 2021

Why onBeforeSnapToItem was removed? I use it with the old version and onSnapToItem reacts a bit too late. Is there an alternative?

@creat-or Use onScrollIndexChanged one.

@anmlkh
Copy link

anmlkh commented Jan 27, 2021

guys, how stable is latest beta? Does it ready to use in production?

@bd-arc
Copy link
Contributor Author

bd-arc commented Jan 27, 2021

@anmlkh We've been using it in production for months, but take it with a grain of salt since the way we use the plugin is not necessarily the same as yours.

What you see is what you get :-)

@ghost
Copy link

ghost commented Feb 8, 2021

In version 4.0.0-beta.6, I have a loop stuck problem on ios, I tried to find the cause in the library and I see now when the stuck list scrollOffSet is not updated causing the problem Trying, checking between nextActiveItem and activeItem is negligible. I skipped the check (nextActiveItem! == this._activeItem) in _onMomentumScrollEnd and everything worked fine. I'm a novice so I can't find out the cause of the bug deeply, someone can and fix it. (react-native 0.63.3)

Same here!

@mstrnisko
Copy link

Been starting to use this recently and noticed the following issues. We run tsc to type check on commit, and adding a dependency to 4.0.0-beta.6 introduced the following typescript errors:

I'm getting the exact same errors(though, only the first two imports from your example). What I ended up doing was putting "skipLibCheck": true, in tsconfig.json

@15110011
Copy link

sorry @bd-arc could you fix this bug #713 on this version?

@SouenMazouin
Copy link

Hi all! 🖖
The typing for disableIntervalMomentum isn't working in 4.0.0-beta.6?

Property 'disableIntervalMomentum' does not exist on type 'IntrinsicAttributes & IntrinsicClassAttributes<Carousel<{ text: string; }>> & Pick<Readonly<CarouselBaseProps<{ text: string; }> & HorizontalCarouselProps<...> & InheritedPropsFromFlatlist<...>> & Readonly<...>, "onLayout" | ... 15 more ... | keyof HorizontalCarouselProps<...>> & Partial<...> & Partial<...>'.

@darkbasic
Copy link

Has development on 4.x stalled?

@Egizas
Copy link

Egizas commented Aug 23, 2021

Looking forward to the v4 of this awesome library!

@ArturoTorresMartinez
Copy link

Anyone knows if there's a status update for this?

@Monir-Shembesh
Copy link

@bd-arc Any release date in mind yet?

@ricardobeat
Copy link

anyone using v4 in production?

@Titozzz seems to be the last committer in the v4 branch a year ago

@BrodaNoel
Copy link

BrodaNoel commented Oct 6, 2021 via email

@bd-arc
Copy link
Contributor Author

bd-arc commented Oct 7, 2021

💫 Production-ready?

We've been using version 4 in production in all our apps for more than a year now, and it's been a game changer!

However it has not been merged yet since some users have been reporting issues that we've never been able to reproduce with our own use cases.

📍 Current state of affairs

Since you guys understandably want an update, you should know that I've been moving away from development, and therefore am no longer maintaining the library.

As you can see in #632, I've been looking for maintainers since December 2019. Unfortunately, no one has really stepped up... yet? I've taken care of the biggest pain point described there with version 4; now it just needs some polish for everyone to enjoy.

Here are the two main things to address in case you're interested 🙂

Please understand that I've spent hundred (if not thousands) of hours since 2016 creating, maintaining, and improving this library. I loved the journey and was very happy to give back to the Open Source community.

But now that I'm done with development for good, the library's in your hands!

Cheers

@NoRamadon
Copy link

thanks @bd-arc for awesome libs,
I have issue with loop on android like video, how can we make it like iOS. The flicker when it reposition in android look ugly.
https://user-images.githubusercontent.com/35259080/140450324-b0bccf86-d738-480b-a06c-60dc31cbc222.mp4

@patissier-boulanger
Copy link

thanks @bd-arc for awesome libs, I have issue with loop on android like video, how can we make it like iOS. The flicker when it reposition in android look ugly. https://user-images.githubusercontent.com/35259080/140450324-b0bccf86-d738-480b-a06c-60dc31cbc222.mp4

same here!

@bilmaijer
Copy link

bilmaijer commented Dec 27, 2021

Hi! I have really simple needs, a swipeable gallery preview with pagination. onSnapToItem wasn't working perfectly. Then I discovered v4 beta. onScrollIndexChanged works like a charm, thanks! 🎉

One tiny problem though, typescript is not happy that the vertical prop is missing, although it shouldn't be required, and by default, the carousel is horizontal anyway, right? (v 4.0.0-beta.6)

Screenshot 2021-12-27 at 13 45 52

@darkbasic
Copy link

v4 typings are non functional, that's one of the issues holding us back from upgrading.

@KirillSocivka02
Copy link

Hi one question, is the package still supported?

@Monir-Shembesh
Copy link

@KirillSocivka02 It is kind of supported i guess? Basically, the main features are set in place as far as I know. There are other features that are still not been implemented but for my personal usage they are not important.

@Aryk
Copy link

Aryk commented Aug 21, 2022

v4 typings are non functional, that's one of the issues holding us back from upgrading.

One example of this is that PaginationProps is not exported so you can't easily extend components...

@demedos
Copy link

demedos commented Sep 27, 2022

Hi one question, is the package still supported?

I wouldn't say so. The last commit in this PR is from July 2020 and the latest stable release is from May 2020.

@buynao
Copy link

buynao commented Jan 3, 2023

It's been two years, is 4.0 still available?

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

Successfully merging this pull request may close these issues.