-
-
Notifications
You must be signed in to change notification settings - Fork 597
fix(Android, Stack): Constraint FormSheet height by top inset #3404
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
base: main
Are you sure you want to change the base?
Conversation
a10f3a4 to
ccfc096
Compare
src/types.tsx
Outdated
| * | ||
| * @platform android | ||
| */ | ||
| sheetShouldOverflowStatusBar?: boolean; |
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 think that we should highlight in the description that with sheetShouldOverflowStatusBar: false, ratios from sheetAllowedDetents will be relative to safe area, not the container height (e.g. if you set sheetAllowedDetents: [0.5], the height (relative to window) will be different depending on sheetShouldOverflowStatusBar prop).
Another question is whether this is the behavior we want.
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.
Another question is whether this is the behavior we want.
Imo it does make sense to calculate detent within the available sheet space. Like, having detent 1.0 and sheetShouldOverflowStatusBar: false would result in content cutoff if we'd calculate maxHeight as detent*containerHeight
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.
Yep. + this way I believe we keep alignment between platforms, which is nice when possible
328f05a to
0646967
Compare
kkafar
left a comment
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.
Looks good overall. I have few comments / requests.
Looking at the final comment from the review: https://github.com/software-mansion/react-native-screens/pull/3404/files#r2545394786 (read first please)
I think that we should treat this as a fix & make the sheets respect insets by default.
Therefore we should work on naming. First, it can not be overflowStatusBar, since it is too specific and not entirely true, right? Doesn't this PR also handle display cutout? Can you verify this? How does it look when there is a big display cutout present (you can turn it on in emulator settings (google for it please))?
If so, we should rather refer to it as topInset IMO.
Additionally if the default's gonna be false, then we should phrase it more like sheetShouldIgnoreTopInset. Then false as the deafult value & if someone wants to active the prop he sets it to true.
What do you think?
android/src/main/java/com/swmansion/rnscreens/bottomsheet/BottomSheetTransitionCoordinator.kt
Outdated
Show resolved
Hide resolved
android/src/main/java/com/swmansion/rnscreens/bottomsheet/BottomSheetTransitionCoordinator.kt
Outdated
Show resolved
Hide resolved
android/src/main/java/com/swmansion/rnscreens/bottomsheet/BottomSheetTransitionCoordinator.kt
Outdated
Show resolved
Hide resolved
...oid/src/main/java/com/swmansion/rnscreens/bottomsheet/BottomSheetWindowInsetListenerChain.kt
Show resolved
Hide resolved
src/types.tsx
Outdated
| * | ||
| * @platform android | ||
| */ | ||
| sheetShouldOverflowStatusBar?: boolean; |
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.
Yep. + this way I believe we keep alignment between platforms, which is nice when possible
guides/GUIDE_FOR_LIBRARY_AUTHORS.md
Outdated
|
|
||
| When set to `false`, the sheet's layout will be constrained by the status bar insets from the top and the detent ratios will then be measured relative to the adjusted height (excluding the top inset). This means that sheetAllowedDetents will result in different sheet heights depending on this prop. | ||
|
|
||
| Defaults to `true`. |
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 in two minds regarding the default value here.
There are a couple of things to consider here:
- what's the previous behaviour, so that we don't introduce breaking changes,
- alignment between platforms,
- reasonable default.
I think that the previous state is that the sheet does expand to full screen height, so that it does not respect the top insets.
As per alignment - iOS does the opposite, it respects the insets (cutout, status bar, etc.).
As per reasonable default - I think it is to respect the insets.
IMO we could treat this as a fix/improvement rather than behaviour change and make it aligned with iOS.
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 think that the previous state is that the sheet does expand to full screen height, so that it does not respect the top insets.
Correct
As per alignment - iOS does the opposite, it respects the insets (cutout, status bar, etc.).
Correct
As per reasonable default - I think it is to respect the insets.
50/50 😄
The only argument I have is to keep the compatibility, because that change will have some impact on the appearance in existing apps, which are having 1.0 detent. Not a strong preference, aligning with iOS seems to be a reasonable approach either.
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 proposing this 5cc9372
I wasn't sure about the support for display cutout, but added it here: 25a543b . Then here: 5cc9372 I'm proposing the new name for the prop. |
kkafar
left a comment
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 have two more remarks. I'll come back to this PR tomorrow with few more questions.
android/src/main/java/com/swmansion/rnscreens/bottomsheet/SheetDelegate.kt
Outdated
Show resolved
Hide resolved
android/src/main/java/com/swmansion/rnscreens/bottomsheet/SheetDelegate.kt
Outdated
Show resolved
Hide resolved
30baf69 to
1cbdabc
Compare
Description
Add a new prop to control formSheet overflow behavior relative to the status bar.
This PR introduces a new prop that allows to control whether a formSheet should overflow the status bar and display cutouts or not. The purpose of this addition is to align the formSheet behavior more closely with iOS, where the top safe area inset is respected by default.
Caution
This is a breaking change that has an impact on the sheet's size (and indirectly may lead to significant visual changes). To bring back the original behavior, you can set
sheetShouldOverflowTopInsetprop totrueFixes https://github.com/software-mansion/react-native-screens-labs/issues/565
Changes
Screenshots / GIFs
Here you can add screenshots / GIFs documenting your change.
You can add before / after section if you're changing some behavior.
With overflow
with-overflow.mov
Without overflow
without-overfow.mov
Test code and steps to reproduce
Test with 3336 with API levels over and below 30
Checklist