Nested Scrollview fix #458
Conversation
|
Can you please provide an e2e test that will prove this correction? |
|
I don't think this is a correct fix. As I understand from the code, this will only scroll the first matched instance. But as the issue describes, if two scroll views exist, outer for vertical and inner for horizontal scrolls, scrolling vertically with this solution would fail. This is a good workaround for people that need the type of solution here, but this is not a general case. Correct me if I'm wrong. |
|
Sorry about not getting back to you until now, but I live on the West Coast of the US (GMT -8) Based on my testing it works, and in theory based on the code it should too. The "_extendToDescendantScrollViews" function gets all UIScrollViews inside of the RCTScrollView and the order they will appear in array will be from parent to child. Since the first UIScrollView that is returned from that array will be the one we're targeting based on a testID the "atIndex(0)" will target the correct one. Unfortunately, I didn't have the time to attack the "waitFor().action().whileElement().scroll()" this time because it uses a great deal of native code to do the whileElement matcher, but this allows testing of nested ScrollViews in iOS in some fashion even if it's not as pretty. |
|
So, I'm not sure what the problem with the 4th build. The first time it froze on the "before all" hook
And the second time it timed out during the lerna set up
Otherwise the tests works, it's a simple nested scrollview that uses "swipe", "scrollTo" and "scroll" to scroll the both the parent and child scroll views. |
|
Hello again, I didn't want to bother anyone around the end of the year holidays, but this passes, there are tests written, is there anything else I can do to complete this pull request? |
|
Looks great, I just need you to make sure it doesn't break Android e2e, and since we still can't run it in CI please make sure it runs locally . When done mention it here and I'll merge |
|
So, I tested android and the way that the deep linking worked there, it was getting stuck. So I just updated the test to launch a non-linked instance so that it wouldn't lock the app in after a device.reloadReactNative() |
|
Hello, checking in again. Seeing where we're at with this. It's all passing. I've verified android locally. Anything else I can do to get this merged? |
|
This update is very important to my project. Can we get an approval please? I have some people on my team looking for another automated testing tool and I would rather stay with detox. |
|
The best way to to have this PR merged is by adding an e2e test that proves its Test app: |
|
Oh, I'm sorry if I placed the tests incorrectly. I added another test suite. (p-nested-scroll-view), linked it to the main page and ran it as a part of the suite. |
| @@ -1,4 +1,7 @@ | |||
| describe('Deep Links', () => { | |||
| after(async () => { | |||
| await device.reloadReactNative(); | ||
| }); | ||
|
|
||
| beforeEach(async () => { |
There was a problem hiding this comment.
You can merge both beforeEachs
| await element(by.text('Nested ScrollView')).tap(); | ||
| }); | ||
|
|
||
| it('Should handle swipe', async () => { |
| await expect(element(by.text('Left'))).toBeVisible(); | ||
| }); | ||
| it('Should handle scroll', async () => { | ||
| try { |
|
Ping ;) |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
The issue has been closed for inactivity. |
|
@Protothor does this work that @rotemmiz can merge in? |
|
Can we please get this PR in? Would be very helpful. |
Solution to issue #164
Gets first scrollView in Element.scroll() to fix iOS nested scrollView problem