fix scroll responder for rn v0.65 / keep backward capatability.

APSL / react-native-keyboard-aware-scroll-view

A ScrollView component that handles keyboard appearance and automatically scrolls to focused TextInput.

MIT License

5.27k stars 646 forks source link

fix scroll responder for rn v0.65 / keep backward capatability. #510

Closed CoryWritesCode closed 2 years ago

CoryWritesCode commented 3 years ago

As stated in #501 that fix isn't backwards compatible with previous versions of RN.

This should keep that compatibility viable while fixing the issue on the latest version of RN.

I created two apps working with different versions of RN to ensure the backwards compatibility. You can view the code for them here

Fixes #508 Fixes #494

CoryWritesCode commented 3 years ago

@slorber does this PR work for you?

mingtsay commented 3 years ago

I have permission to merge and publish this package, but can't dedicate time to it.

Who has used the fork, and can confirm it works for both older versions of RN + 0.65?

Merging a change that only works for the new RN version would harm the community. What is the current RN support of current lib version? When was scrollTo introduced exactly?

Please make sure that it's working for you (on RN 0.65) but for others too, and invest your own time to convince me it's the case with a demo app or whatever, because I can't blindly merge this PR nor invest my own time atm.

@slorber You might want to see this PR. It will use scrollTo when the scrollResponderScrollTo is not available. That will cover all legacy versions and support the versions from 0.65.

jcgertig commented 2 years ago

@slorber @mingtsay please any movement on this would be great

mingtsay commented 2 years ago

I tested in 0.66 and it works. So no worry for the latest stable version. 🥂

CoryWritesCode commented 2 years ago

@bmatasar I added @mlazari's checks into the PR. Thank you @mlazari!!

scblason commented 2 years ago

Any timeline in when this will be merged? Needed ASAP

safaiyeh commented 2 years ago

@slorber could we get some eyes on this?

finalquest commented 2 years ago

Hi guys, I don't wan't to push on this. But, as today November 1, android only supports api level 30. To make that level work, we need to update RN to at least 0.65.0. This issues will prevent a great number of devs to update. Can we make a release with this fix ASAP.

Regards

Noitham commented 2 years ago

Hi guys, I don't wan't to push on this. But, as today November 1, android only supports api level 30. To make that level work, we need to update RN to at least 0.65.0. This issues will prevent a great number of devs to update. Can we make a release with this fix ASAP.

Regards

You can bumpcompileSdkVersion/targetSdkVersion without necessarily upgrading react-native. (currently on 63.3 with API level 30)

mantegnous commented 2 years ago

So, when merge ?

slorber commented 2 years ago

Released in 9.5, thanks for the retro compatible change @CoryWritesCode and sorry for the delay

To everyone asking "please merge, I need this ASAP!!!", you are hurting the open-source community and actually delayed the merge of this PR:

you add noise, making it harder to review good feedbacks and reviews from real contributors
you make maintainers feel guilty and burn out, maintainers that you don't pay and owing to you nothing, leading to abandoned projects
it's easy to use a RN fork as a dependency (or patch-package), and if you NEED this PR, you can only blame yourself
this PR doesn't pass yarn test due to eslint issues => surprising for a PR that was considered ready to merge by everyone

adias9 commented 2 years ago

Thanks @slorber for being responsive on this issue. I'm sorry many flooded this thread with noise. Really grateful for your work. ❤️ I think many people love this package and are going about expressing their love and the critical use for this package in an unfriendly way. If I may speak for them, we are super grateful that this package exists. I know it has helped in my team's dev time.