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

Consider to use FlatList. #73

Closed
scerelli opened this issue May 22, 2017 · 26 comments
Closed

Consider to use FlatList. #73

scerelli opened this issue May 22, 2017 · 26 comments

Comments

@scerelli
Copy link

scerelli commented May 22, 2017

Did you consider to use FlatList instead of ScrollView? it will improve a lot performance and it has lazy load for items that works properly

@scerelli scerelli changed the title Consider to use flatlist. Consider to use FlatList. May 22, 2017
@bd-arc
Copy link
Contributor

bd-arc commented May 22, 2017

Hi @scerelli, and thanks for the suggestion.
In fact, yes, we have considered this. Unfortunately, at this point, issues like this one are a no-go for us.

Still, we'll continue to experiment with FlatList and watch the component's evolution closely.

@PublicParadise
Copy link

Hello folks,
@bd-arc just brought this thread to my attention.

I am interested in using react-native-snap-carousel with large number of items (~2000). From what I understand FlatList is still a bit immature and has bugs like facebook/react-native#14037 that are pretty much show stoppers.

What do you guys think getting started with a port to ListView first?
Facebook's documentation for ScrollView reads:

ScrollView works best to present a small amount of things of a limited size. All the elements and views of a ScrollView are rendered, even if they are not currently shown on the screen. If you have a long list of more items that can fit on the screen, you should use a ListView instead.

It seems that switching to ListView first would already address some of those ugly performance problems I am currently seeing with large number of items in react-native-snap-carousel.

It also looks like FlatList can almost be used as a drop-in replacement for ListView. One would hope that Facebook will eventually fix those FlatList bugs. In that case we could potentially painlessly swap in FlatList for ListView whenever we feel FlatList had finally reached production quality.

If you guys agree with this direction would you be interested in starting a new branch that ports react-native-snap-carousel over to ListView first?

@bd-arc
Copy link
Contributor

bd-arc commented Jun 2, 2017

@PublicParadise Note that there is one major drawback with using either ListView or FlatList: one would no longer be able to use different components as carousel's slides, which can ruin the plugin for some users...

With that in mind, it would be great to offer the best of both worlds:

  • ScrollView for shiny carousels with miscellaneous slides' types and less than, for example, 50 items
  • FlatList for performance-oriented carousels with identical items.

I need to play with the code to see how maintainable that would be.

Ideas and suggestions are very welcome ;-)

@bd-arc bd-arc reopened this Jun 2, 2017
@bd-arc
Copy link
Contributor

bd-arc commented Jun 2, 2017

@PublicParadise Well, forget about what I just said; the prop getItemLayout of FlatList should really help smoothing things out. We might even be able to provide support for children with different itemWidth.

So let's focus on migrating to Flatlist in the medium term ;-)

@PublicParadise
Copy link

@bd-arc I like the idea of offering both worlds. You could always decide later if you want to retire the ScrollView version.

@EdmundMai
Copy link

hi do you have any clue when this will be finished? really looking forward to the flatlist version!

@bd-arc
Copy link
Contributor

bd-arc commented Jul 5, 2017

Hi @EdmundMai, I should be able to work on it this week or the the week after. Stay tuned!

@EdmundMai
Copy link

@bd-arc awesome, thanks so much for this library!!

@tslater
Copy link

tslater commented Jul 11, 2017

Is facebook/react-native#14037 still an issue? It looks like it was closed for lack of data? Does a throttle fix it sufficiently?

@k2glyph
Copy link

k2glyph commented Jul 17, 2017

I am also having perfomance issues with this library. Please use flatlist.

@bd-arc
Copy link
Contributor

bd-arc commented Jul 24, 2017

Ok guys, I've got some good news: FlatList's implementation is just a matter of hours now.

Currently checking if something obvious has been broken and implementing a few things, and then it's yours to try ;-)

@bd-arc
Copy link
Contributor

bd-arc commented Jul 24, 2017

And here we are! I've just pushed a flatlist branch with all the relevant commits.

I've tested it thoroughly in real projects and it seemed pretty good to me (even though FlatList is still full of bugs). I've also added a new prop activeSlideAlignment (see #3), since it was pretty easy to implement with FlatList.

Now here is the deal: I need feedback on this update. Be aware that I won't publish it until I've made sure that it hasn't broken anything serious.

To make testing easier, the branch includes an updated README as well as an updated example ;-)

flatlist example

@mazing
Copy link

mazing commented Jul 26, 2017

It works very well with FlatList! Thanks!

However, if data array is empty, it throws the error scrollToIndex out of range: 0 vs -1 where 0 is the index of the current slide.

Looking forward to the release!

@bd-arc
Copy link
Contributor

bd-arc commented Jul 26, 2017

Thanks for noting this issue @mazing!

I never initialize my carousels while data hasn't been fetched, which is why I tend to overlook this kind of error. I'll shortly push a fix.

@jarvisluong
Copy link

Hi!

How can I install this flatlist version by using npm? I would like to be a little white mouse to experiment this feature :-)

@scerelli
Copy link
Author

yes just use the GitHub link instead of version and point to the flatlist branch using #

@bd-arc
Copy link
Contributor

bd-arc commented Jul 30, 2017

Hi @jarvisluong,

@scerelli said it all :-) To get the current latest commit of the flatlist branch, and unless I'm mistaken, you would use the following:

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

Don't forget to check the updated documentation as well as the migration guide.

Looking forward to your feedback ;-)

@bd-arc
Copy link
Contributor

bd-arc commented Aug 1, 2017

Hi guys,

Here comes a pretty neat feature I've been working on during the past few days because I needed it for a project: parallax images, native driver-powered 🍾

react-native-snap-carousel parallax image

As usual, documentation and example have been updated. Let me know what you think!

react-native-snap-carousel parallax images

@jarvisluong
Copy link

So far the FlatList version is working marvelously 👍 . The performance really bumped up!

@bd-arc
Copy link
Contributor

bd-arc commented Aug 2, 2017

Thanks for the feedback @jarvisluong! I've squashed a few bugs today but yes, the new implementation seems to work as expected.

I'm now going to work on implementing a proper loop mode. If no major issue has been reported since then, it will probably be a good time to release version 3.0.0 ;-)

@mazing
Copy link

mazing commented Aug 2, 2017

I had some troubles updating to react-native v0.47.0.

First, I received errors about react-native-snap-carousel, so I uninstalled react-native-snap-carousel and installed react-native@0.47.0 and re-installed react-native-snap-carousel. But now the items are not shown in the carousel, but I don't receive any errors.

@bd-arc
Copy link
Contributor

bd-arc commented Aug 2, 2017

Hey @mazing,

I've just updated the example to RN v0.47.1 (see the latest commit) and didn't have any issue. Would you mind trying it and letting me know if you still have troubles?

@mazing
Copy link

mazing commented Aug 4, 2017

I'm sorry - my bad! When I reinstalled, I forgot to install the flatlist branch, so the items were simply not shown, since I only had a renderItem property, which wasn't used in the old version.

@mazing
Copy link

mazing commented Aug 4, 2017

I seem to have problems using

componentDidMount() {
  this.props.navigation.setParams({
    key: 'value'
  });
}

in react-navigation at screens with <Carousel ... />.

I receive the error

screen shot 2017-08-04 at 10 16 10

@bd-arc
Copy link
Contributor

bd-arc commented Aug 4, 2017

@mazing Do you mind copying/pasting your latest comment into a dedicated issue? I think it might have to do with your setup and not with the FlatList implementation ;-)

@bd-arc
Copy link
Contributor

bd-arc commented Aug 23, 2017

Version 3.0.0 has been published!

@bd-arc bd-arc closed this as completed Aug 23, 2017
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

8 participants