-
-
Notifications
You must be signed in to change notification settings - Fork 571
fix(Android): Differentiate between new and preloaded screens in ScreenStack #3062
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(Android): Differentiate between new and preloaded screens in ScreenStack #3062
Conversation
400fef5
to
a8f818a
Compare
I wonder how this solution relates to #2945 |
Looks like both solve the same problem and work with the examples. The flag is less elegant but more local, and the second one might break something else. |
isBeingActivated
flag to differentiate between new and preloaded screens@@ -148,7 +149,7 @@ class ScreenStack( | |||
var shouldUseOpenAnimation = true | |||
var stackAnimation: StackAnimation? = null | |||
|
|||
val newTopAlreadyInStack = stack.contains(newTop) | |||
val newTopAlreadyInStack = stack.contains(newTop) && !preloadedWrappers.contains(newTop) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should add a comment explaining new semantics of newTopAlreadyInStack
.
preloadedWrappers = screenWrappers | ||
.asSequence() | ||
.filter { it.screen.activityState == Screen.ActivityState.INACTIVE } | ||
.toList() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure here if all screens with inactive state are preloaded. Please add a comment abot this assumption here.
apps/src/tests/Test3004b.tsx
Outdated
flexDirection: 'column', | ||
alignItems: 'center', | ||
justifyContent: 'center', | ||
backgroundColor: 'rgba(0.2, 0.2, 0.4, 0.3)' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where possible, you can use colors from apps/src/shared/styling/Colors.ts
apps/src/tests/index.ts
Outdated
export { default as Test3004a } from './Test3004a'; | ||
export { default as Test3004b } from './Test3004b'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These test names won't work with current sorting logic in example app. You can try using 3004.1
& 3004.2
or use issue number and PR number.
@@ -303,6 +304,7 @@ class Screen( | |||
} | |||
|
|||
fun setActivityState(activityState: ActivityState) { | |||
Log.d("RNScreens", "Activity state updated to %s".format(activityState.toString())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remember to remove logs and redundant imports. Applies also to ScreenStack.kt
apps/App.tsx
Outdated
return <Example />; | ||
// return <Test.Test3004_1 />; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to leave this file as it was if Example app does not work properly without Reanimated/GestureHandler.
@@ -148,7 +150,8 @@ class ScreenStack( | |||
var shouldUseOpenAnimation = true | |||
var stackAnimation: StackAnimation? = null | |||
|
|||
val newTopAlreadyInStack = stack.contains(newTop) | |||
// We don't count preloaded screen as "already in stack" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd consider adding more details why this is the case. You can also link to this PR if you think it would help.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rest seems good
apps/src/tests/Test3004_2.tsx
Outdated
} | ||
|
||
function Modal({ navigation }: StackNavigationProp) { | ||
return (<View style={{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small nitpick, wouldn't it be more readable to move parentheses to separate like in *_1
example?
@@ -150,7 +149,8 @@ class ScreenStack( | |||
var shouldUseOpenAnimation = true | |||
var stackAnimation: StackAnimation? = null | |||
|
|||
// We don't count preloaded screen as "already in stack" | |||
// We don't count preloaded screen as "already in stack" up until it appears with state == ON_TOP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// We don't count preloaded screen as "already in stack" up until it appears with state == ON_TOP | |
// We don't count preloaded screen as "already in stack" until it appears with state == ON_TOP |
ac1f77e
to
f15955e
Compare
Description
Closes #3004.
When
onUpdate
is issued for ScreenStack, we have no way of knowing whether the screen that is on top with state=2 was already on top or was preloaded with state=0, so we need to store this information somehow, i.e in a list.Changes
Added
preloadedWrappers
list toScreenStack
class and update it at the end of eachonUpdate
.Test code and steps to reproduce
See Test3004