-
-
Notifications
You must be signed in to change notification settings - Fork 518
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
feat: add headers to modal #821
Conversation
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.
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.
Left some comments but looks good overall 🙌 Good job! 🎉
index: number, | ||
descriptor: NativeStackDescriptor | ||
) => { | ||
if (isHeaderInModal) { |
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.
This may be problematic if this option changes dynamically as it'll unmount the previous component and render new one, and all state will be destroyed.
any news on this pr? I am looking also for this feature 👍 |
Hopefully this will be merged soon. You can always patch-package these changes into your project to see how it works and give us feedback. |
any update? |
if ( | ||
!didWarn && | ||
stackPresentation !== 'push' && | ||
headerShown !== undefined | ||
) { | ||
didWarn = true; | ||
console.warn( | ||
'Be aware that changing the visibility of header in modal on iOS will result in resetting the state of the screen.' | ||
); | ||
} |
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.
@WoLewicki Would it not make more sense to warn about this whenever a visibility change actually happens?
Now I get a warning every time I run the app and open a modal despite having a static headerShown: false
that will never cause this issue.
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 would need to keep track of the previous value of this prop in order to do that, and I am not sure if it is a good idea. Also, if you do not want the header to be shown, you can just not change the prop in modal screens at all. You can easily remove the warning by using LogBox.ignoreLogs
.
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.
Yeah, good point.
I see it's triggered due to screenOptions={{ headerShown: false }}
on the navigator. I know you suggest setting headerShown
on each screen instead, but why is that suggested exactly?
You can easily remove the warning by using LogBox.ignoreLogs.
headerShown: undefined
on the modal screen works as well, as it seems to override the navigator's headerShown
.
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.
Yeah, setting props on individual screens override the options set in navigator's screenOptions
.
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 understand, then basically I can ignore the warning if I did not intentionally change headerShown
prop. Just to make sure I am not doing anything wrong by paying attention to all warnings. :)
Added option for achieving headers in modal on iOS. The change is based on adding additional ScreenStack to every modal Screen component and adding the HeaderConfig component to upper or lower Screen based on the stackPresentation to ensure proper header transitions. Modal headers will not have the back button on iOS, since the header comes from the nested stack there and has no previous screen.
Description
Added option for achieving headers in modal on iOS. The change is based on adding additional
ScreenStack
to everymodal
Screen
component and adding theHeaderConfig
component to upper or lowerScreen
based on thestackPresentation
to ensure proper header transitions. Modal headers will not have the back button on iOS, since the header comes from the nested stack there and has no previous screen. Requested in #800.Changes
Changed
NativeStackView.tsx
to contain nestedScreenStack
with properHeaderConfig
component manipulation. Same with v4native-stack
.Test code and steps to reproduce
Test800.tsx
inTestsExample
. It will require to test many scenarios in order to make sure it doesn't break anything.Checklist