oblador / react-native-collapsible

Animated collapsible component for React Native, good for accordions, toggles etc
MIT License
2.47k stars 452 forks source link

feat: Replace container View with FlatList #318

Closed timhagn closed 3 years ago

timhagn commented 5 years ago

As I was using this in an App and an Accordion with 100 Items took around 3s to display, I:

I'm eager to get this merged, as I'm using this excellent package in a (soon to be) production app and would like to stop using a copied version (no GitHub linking allowed through corporate proxy).

Best,

Tim. P.S.: Would solve #291 and (probably) #305 : ).

EDIT: Typo.

timhagn commented 5 years ago

Hi @iRoachie,

thanks for the Review : ). Would happily add this to the example app - but probably would have to adapt it some more. Am using this currently in an App at a Gig (therefore my late reply % ) and ran into a problem with a reusable component with split data (by date) which I had to put within a ScrollView. And of course RN now gives me warnings about "VirtualizedList in a ScrollView" - so probably both the FlatList (for speed gains) as well as the "simple" View (for the case mentioned) based versions should be possible. Perhaps by adding a switch to be more downwards compatible? Gonna have to refactor my version of it anyhow ^^.

Best,

Tim.

timhagn commented 5 years ago

Hi @iRoachie,

meanwhile I added the switch renderAsFlatList defaulting to false for legacy behavior and extracted rendering of the main Container into it's own function. Works flawlessly in my (sadly corporate, thus more than private) repo at work. I also updated the type definitions from Definitely Typed, but to be able to use all options of FlatList more work needed to be done, which I don't have the time for, at the moment : /. Especially with TS I'm just beginning my journey...

Hopefully this will get merged anyway, cause it won't break anything as it's downwards compatible : ).

Best,

Tim.

EDIT & P.S.: Just tried to test the changes in the example App, but this would be another matter all together - Expo SDK 30 isn't even supported anymore...

iRoachie commented 5 years ago

I don't think we need to have a switch (renderAsFlatList). This should be a replacement for the current behaviour. Let's keep complexity down and just have the flat list implementation.

timhagn commented 5 years ago

As already mentioned in my comment three days ago, there are use cases (for me in the same App) where a View is needed instead of a FlatList: encapsulating (multiple) Accordions inside a ScrollView. To be able to use it in both cases I added the switch.

iRoachie commented 5 years ago

I'm not seeing any of these errors you mention when nesting a flatlist with a scrollview https://snack.expo.io/@roach_iam/thankful-truffles

timhagn commented 5 years ago

Ah, hadn't seen that you already updated the example app, verified it with an updated version on my own ^^. Just a small change to it was needed (the values in SELECTORS had to be strings) to make the buttons work with a FlatList. The mentioned error is an annoying warning when using it with an app bootstrapped from react-native-cli or ejecting it from expo. It also sped things up a little for me (seemingly a FlatList needs more memory than a View).

TheTimeWalker commented 4 years ago

Any progress in this? I have been using timhagn's fork for quite a while and for my usage it seems to work perfectly fine in my use case.

The only real issue I see is that there's no way to set the keyExtractor property. It seems like the key extraction from Flatlist doesn't play nice with this as I always get a warning that each item has to have an unique key.

lukecurtis93 commented 4 years ago

any update on this? i'm finding the performance taking a bit of a hit without a flat list right now with complex information inside my components

timhagn commented 4 years ago

Yup, I'd love to have this merged - am using it in a corporate environment and would prefer to use the "official" component. @TheTimeWalker What's your keyExtractor() looking like? As I have no trouble with the existing implementation in my fork. I'd be happy to help : ).

Let's get this merged! Or what should I change @iRoachie? Cause you not seeing any warnings in Expo doesn't mean they don't exist with react-native-cli based projects ; ).

ryanphung commented 4 years ago

I'm using this component. Big THANK YOU to the team! Great job thoroughly.

However I also run into similar performance issue. Hope this will be merged soon! Thank you for your work @timhagn . Meanwhile probably I'll use your fork!

mattmattvanvoorst commented 4 years ago

Using @timhagn's fork, hoping this gets merged soon

RWOverdijk commented 4 years ago

Oblador seems to be MIA on all repos.

LucasGarcez commented 4 years ago

Hi people, I need this implementation, someone can please merge this?

timhagn commented 4 years ago

Hi there,

I would provide a forked npm package of this one, but am to booked right now, to fix the other open issues % ). Happy that my fork helped so many people and it's sad, that this still isn't merged : /.

Best,

Tim.

2xSamurai commented 4 years ago

Hi, Is this going to be merged soon? Thanks for the great plugin.

mattmattvanvoorst commented 4 years ago

@timhagn At this point, you might want to consider offering your own package?

zaptrem commented 4 years ago

@timhagn Since this is now rendering as a FlatList what is the correct way to tap into FlatList callbacks like onEndReached for infinite scroll?

jyepesr1 commented 4 years ago

@timhagn is there any npm package that you have published with your version?

Relax594 commented 4 years ago

@oblador @iRoachie we need this so bad for performance reasons.

timhagn commented 4 years ago

Now, as nearly a year has passed since I opened this PR (and I finally have some time on my hands ; ), I'm gonna try publishing my own package from my (then updated) fork in the next few days % ).

ryanphung commented 3 years ago

Hi @timhagn , just dropping by to say hi! Also hope you'll manage to find some spare time to get around with publishing your own package :-)

For folks who are still waiting for the npm package, this is how you can directly use the git version in your package.json. Not the best way but at least it works for me:

"react-native-collapsible": "git+https://github.com/timhagn/react-native-collapsible.git#b8dba7f"
timhagn commented 3 years ago

I know, I know, still open - but when one gets a new (paid ; ) project nearly direct after writing the last comment, which has nothing to do with RN... Please bear with me % ).

mattmattvanvoorst commented 3 years ago

@timhagn for once I welcome receiving an email for a package I once commented on half a year ago - welcome back!

timhagn commented 3 years ago

Hi @mattmattvanvoorst & all others eagerly waiting!

hmmz Do I err, or does this repo / package from @JoaoPauloCMarra look a lot like my code having been copied into react-native-collapsible with a few additions more: react-native-collapsible-updated oO?

Especially when looking at Accordion.d.ts the wording is awfully similar ^^...

Mine:

  /**
   * Render the Accordion as a FlatList. Defaults to false for legacy behavior.
   *
   * @default false
   */

The other ones:

  /**
   * Render the Accordion as a FlatList. Defaults to false for legacy behavior.
   * plus FlatList props
   *
   * @default false
   */

Doesn't annoy me (much ; ), as I didn't work with RN since October and would now first have to get back in (should that package work for you all that is), a little "attribution" would just have been nice & thus a direct merge & no copy...

Or do only I see it like that?

Best,

Tim.

oblador commented 3 years ago

Thanks for your hard work and the long wait! Had to do a few changes to not break backwards compatibility 👍

timhagn commented 3 years ago

Hi @oblador,

woohoo! Had nearly forgotten this one, thanks for the merge : )!

Best,

Tim.