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

fix: use animated reaction instead of derivation #255

Merged
merged 2 commits into from
Oct 14, 2023

Conversation

vonovak
Copy link
Contributor

@vonovak vonovak commented Oct 6, 2023

📜 Description

💡 Motivation and Context

this is more or less a stylistic change, the useAnimatedReaction hook is better suited for this than useDerivedValue which is supposed to derive a value, but not have side effects

📢 Changelog

not needed I guess?

🤔 How Has This Been Tested?

I ran this on an Android 10 device (huawei) and it works the same as before.

I used the 'Aware scroll view' example to test this out and I have to say that, unfortuantely, on my device the animation is not (and was not) smooth. That's a different problem though, I might try to open a PR for that later.

📸 Screenshots (if appropriate):

📝 Checklist

  • CI successfully passed

@kirillzyusko
Copy link
Owner

Good job @vonovak!

May I ask you to make the same changes in FabricExample as well?

I know it's bad practice to have code duplication and I want to have a single code for example apps, but for now I have to duplicate it 😔

I have to say that, unfortuantely, on my device the animation is not (and was not) smooth. That's a different problem though, I might try to open a PR for that later.

Did you (or maybe you can) test it on Android 11+? And on which RN version are you testing this example?

On Android < 11 animations are not perfectly synchronized. And I noticed that on RN> 0.72 some examples that were relying on height animations became laggy 🤔 I wanted to investigate why but didn't have time for that (and I still don't understand whether it's a problem of RN or new REA version).

@kirillzyusko kirillzyusko self-assigned this Oct 6, 2023
@kirillzyusko kirillzyusko added the example Anything related to example project label Oct 6, 2023
@vonovak
Copy link
Contributor Author

vonovak commented Oct 6, 2023

Hello, yes, I'll update it too on Monday 😉.

And test on newer Android.

I tested directly on this branch so whatever the example is running now.

Here's a recording from my device. It is visibly "jumpy".

SVID_20231006_120803_1.mp4

@kirillzyusko
Copy link
Owner

Yes @vonovak

But I see, that only back transitions are very laggy. When keyboard appears the animation is pretty smooth, right?

@vonovak
Copy link
Contributor Author

vonovak commented Oct 6, 2023

That is correct @kirillzyusko 👍

@kirillzyusko
Copy link
Owner

@vonovak really strange 🤔 On my device (Pixel 7 Pro, Android 13) everything is pretty smooth:

telegram-cloud-document-2-5260428862709315175.mp4

And on my Redmi Note 5 Pro with Android 9 it's also pretty smooth 🤔

Maybe there is a conflict between height animation and calling scrollTo 🤷‍♂️ If you can reproduce this problem on emulator - let me know a configuration and I'll try it on my end 👍

@vonovak
Copy link
Contributor Author

vonovak commented Oct 6, 2023

Eh, different day, same crap to deal with - android 🫣😄.

Okay, I'll see if I can repro.

@vonovak
Copy link
Contributor Author

vonovak commented Oct 14, 2023

@kirillzyusko I pushed the changes to fabric example. Regarding the repro, I ended up doing things differently and not sure I'll have time to create it, sorry about that.

@kirillzyusko kirillzyusko merged commit 8a33ffd into kirillzyusko:main Oct 14, 2023
kirillzyusko added a commit that referenced this pull request Jan 13, 2024
…ScrollView` (#332)

## 📜 Description

Synchronized bottom padding with keyboard frame in
`KeyboardAwareScrollView`.

## 💡 Motivation and Context

Original regression was introduced in
#257
([these](https://github.com/kirillzyusko/react-native-keyboard-controller/pull/257/files#diff-b4493213c8dc470b5ba251af896d133130cc8996cee1118525e4eba1f3aa993bL87-L88)
code lines). I decided not animated bottom padding to potentially fix
the issue described in
#255 (comment)

However such approach is causing unsynchronized animations in certain
conditions and I'm not sure, that it actually fixes the problem
described in comment above.

So in this PR I'm reworking it again and bringing back animated
transitions 👀

Closes
#329

## 📢 Changelog

### JS

- added `currentKeyboardFrameHeight` variable;
- update `currentKeyboardFrameHeight` variable in `onMove` handler;
- use `currentKeyboardFrameHeight` for bottom padding in KASV.

## 🤔 How Has This Been Tested?

Tested manually on iPhone 15 Pro + e2e tests.

## 📸 Screenshots (if appropriate):

|Before|After|
|-------|-----|
|<video
src="https://github.com/kirillzyusko/react-native-keyboard-controller/assets/22820318/a7e5e3d3-8ef0-4b69-8c21-2414b1899e91">|<video
src="https://github.com/kirillzyusko/react-native-keyboard-controller/assets/22820318/bdd34dee-25bd-4491-8612-6048c88760bb">|

## 📝 Checklist

- [x] CI successfully passed
- [x] I added new mocks and corresponding unit-tests if library API was
changed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
example Anything related to example project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants