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: check if activityState is nil #714

Merged
merged 4 commits into from Nov 19, 2020
Merged

Conversation

WoLewicki
Copy link
Member

@WoLewicki WoLewicki commented Nov 17, 2020

Description

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. Fixes #702.

Changes

Added check for nil value of activityState. Added new prop activityStateOrNil. Changed logic in ScreenContainer's updateContainer.

Test code and steps to reproduce

Test702.js file with reproduction in TestsExample.

Checklist

  • Included code example that can be used to test this change
  • Ensured that CI passes

@dylancom
Copy link
Contributor

@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.

@WoLewicki
Copy link
Member Author

@dylancom can you provide a reproduction that shows this?

@dylancom
Copy link
Contributor

@WoLewicki https://github.com/dylancom/react-native-screens-animation-bug
The same branch as I provided already, contains the modal setup. So if you open the modal with the "Open Modal" button.

@WoLewicki
Copy link
Member Author

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.

@dylancom
Copy link
Contributor

@WoLewicki Oops sorry, thought the 2nd file only had spacing changes. Seems to work now!

Copy link
Member

@kmagiera kmagiera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@kmagiera
Copy link
Member

Also the PR title does not look too descriptive. Can you update the title to better reflect what we are changing here?

@WoLewicki WoLewicki changed the title fix: add check for nil fix: check if activityState is nil Nov 19, 2020
Comment on lines 34 to 35
// 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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// 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.

@WoLewicki WoLewicki merged commit 9358d45 into master Nov 19, 2020
@WoLewicki WoLewicki deleted the @wolewicki/nil-activity-state branch November 19, 2020 11:26
WoLewicki added a commit that referenced this pull request Jan 20, 2021
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Push animation between screens not working
3 participants