fix: check if activityState is nil #714
Conversation
|
@WoLewicki it brings back the animation, but it seems that my modal screens lose interactivity. Once I open a modal, I can't press any button to go out of it anymore. |
|
@dylancom can you provide a reproduction that shows this? |
|
@WoLewicki https://github.com/dylancom/react-native-screens-animation-bug |
|
I do not have such an issue. Did you apply changes from both files? I test in in https://github.com/software-mansion/react-native-screens/blob/master/TestsExample/src/Test702.js with changes applied. |
|
@WoLewicki Oops sorry, thought the 2nd file only had spacing changes. Seems to work now! |
There was a problem hiding this comment.
Code looks good. However, I'd like to place some additional comments explaining why we have this unusual handling added. Accepting to unblock but please explain the meaning of nil and circumstances when it may occur in the code that handles it prior to merging.
| @@ -58,9 +58,10 @@ - (void)updateBounds | |||
| [_bridge.uiManager setSize:self.bounds.size forView:self]; | |||
| } | |||
|
|
|||
| - (void)setActivityState:(int)activityState | |||
| - (void)setActivityStateOrNil:(NSNumber *)activityStateOrNil | |||
There was a problem hiding this comment.
Would be great to document it somewhere in the code the purpose of special handling nil value and also explain when nil can be provided. That is we should mention that nil will be provided when activityState it is set as an animated value and we change it from JS to be a plain value (non animated). I understand that in this scenario we first trigger the code which resets it to the default, which is nil, and which maps to 0 meaning Inactive. In that case we want to ignore such update as soon after we get the actual state passed from JS.
|
Also the PR title does not look too descriptive. Can you update the title to better reflect what we are changing here? |
WoLewicki
force-pushed
the
@wolewicki/nil-activity-state
branch
from
e09b48a to
98efbe9
Compare
| // In this scenario we first trigger the code which resets it to the default, which is null. | ||
| // In that case we want to ignore such update because soon after we get the actual, valid state passed from JS. |
There was a problem hiding this comment.
| // In this scenario we first trigger the code which resets it to the default, which is null. | |
| // In that case we want to ignore such update because soon after we get the actual, valid state passed from JS. | |
| // In case when null is received we want to ignore such value and not make | |
| // any updates as the actual non-null value will follow immediately. |
Changed the logic of resolving activityState to take into account the initial nil value of it by passing the NSNumber instead of int (which was resolved to 0) on iOS. The logic for restoring user interaction in ScreenContainer had to be changed too since it did not take into account going from value 1 to 2 for one of Screens. It worked before because, at the end of the transition, the activityState of all Screens was changed to 0 due to the initial nil pass, and then switched back to the proper value in the next evaluation, which resulted in screenAdded being set to YES. A similar situation, but without the need for change in ScreenContainer applies to Android.
Description
Changed the logic of resolving
activityStateto take into account the initialnilvalue of it by passing theNSNumberinstead ofint(which was resolved to0) on iOS. The logic for restoring user interaction inScreenContainerhad to be changed too since it did not take into account going from value1to2for one ofScreens. It worked before because, at the end of the transition, theactivityStateof allScreens was changed to0due to the initialnilpass, and then switched back to the proper value in the next evaluation, which resulted inscreenAddedbeing set toYES. A similar situation, but without the need for change inScreenContainerapplies to Android. Fixes #702.Changes
Added check for
nilvalue ofactivityState. Added new propactivityStateOrNil. Changed logic inScreenContainer'supdateContainer.Test code and steps to reproduce
Test702.jsfile with reproduction inTestsExample.Checklist