fix: remove unnecessary checks for creating snapshot #2485
Conversation
| screenChildComponent == [_presentedModals.lastObject view])) { | ||
| [screenChildComponent.controller setViewToSnapshot]; | ||
| } | ||
| [screenChildComponent.controller setViewToSnapshot]; |
There was a problem hiding this comment.
I see that this simplifies the logic, but I don't think this is quite right.
Imagine you have Stack with three screens: "A, B, C". You pop from C to top, thus resulting stack is "A". This code would do snapshots of both C and B, while we only want to do a snapshot of C.
If you describe what problem exactly you intend to solve we might be able to figure out better solution.
There was a problem hiding this comment.
Will this code:
react-native-screens/ios/RNSScreen.mm
Line 1842 in 4614702
true for B? I think its view is not in the window since it is detached. Would have to check though.
There was a problem hiding this comment.
I checked. It is nil then so it should work correctly.
There was a problem hiding this comment.
+1, I've forgotten that UINavigationController will keep all the controllers lower in the stack in detached state and that's it why it will work correctly.
| break; | ||
| default: | ||
| RCTLogError(@"[RNScreens] Unhandled subview type: %ld", childComponentView.type); | ||
| if (childComponentView.window != nil) { |
There was a problem hiding this comment.
Also added this check since we got many warning of snapshotting a view that is not in hierarchy when dismissing many screens with header items at once.
| screenChildComponent == [_presentedModals.lastObject view])) { | ||
| [screenChildComponent.controller setViewToSnapshot]; | ||
| } | ||
| [screenChildComponent.controller setViewToSnapshot]; |
There was a problem hiding this comment.
+1, I've forgotten that UINavigationController will keep all the controllers lower in the stack in detached state and that's it why it will work correctly.
|
I've restarted the CI & we need to update the podfile lock. |
Description
Based on Expensify/App#49937 (comment) and the comments above, PR fixes the unnecessary checks for creating snapshot on the view on dismiss. It was refactored already in #2134 and #2261 (👏 to @kkafar). Those checks do nothing now since each screen is responsible for making its own snapshot. Having those checks can only lead to problems.