react-native-screens
react-native-screens copied to clipboard
feat(iOS): support for custom detents with `formSheet` presentation
Description
This PR adds support for custom detents with formSheet
modal presentation on iOS >= 16.0
.
This new feature can be used with native-stack v5
(react-native-screens/native-stack
) via sheetAllowedDetents
prop by passing an array of values to it (see prop description in docs).
I plan on exposing it in v6 (@react-navigation/native-stack
) in https://github.com/react-navigation/react-navigation/pull/11516 or in a follow up PR.
Note: I'm looking for any suggestions on better prop naming :D
See #1870 for view flickering issue description & solution
Resolves #1772
Changes
- [x] Implemented native logic
- [x] Exposed the prop from native side
- [x] Exposed the prop and set default values in JS
- [x] Added Android stubs
- [x] Updated docs
Implementation notes
Please look below for my review.
Known issues
View flickering on Paper and Fabric (the exact cause is not determined yet, but the code responsible for this is in updateBounds
method which is called on every layout recalculation and it reports current view size to react (/ updates shadow node).
See:
- https://github.com/software-mansion/react-native-screens/issues/1722
Test code and steps to reproduce
Test1649
in both TestsExample
and FabricTestExample
apps.
Checklist
- [x] Included code example that can be used to test this change
- [x] Updated TS types
- [x] Updated documentation:
- [x] https://github.com/software-mansion/react-native-screens/blob/main/guides/GUIDE_FOR_LIBRARY_AUTHORS.md
- [x] https://github.com/software-mansion/react-native-screens/blob/main/native-stack/README.md
- [x] https://github.com/software-mansion/react-native-screens/blob/main/src/types.tsx
- [x] https://github.com/software-mansion/react-native-screens/blob/main/src/native-stack/types.tsx
- [x] Ensured that CI passes
Do you want some help to test this @kkafar ? Also, if you make this native for Android... you will be a hero 😅.
Hey @ferrannp, thank you and yes -- I would really use a hand in testing this stuff, any feedback is welcome.
Currently I'm working on Fabric implementation which has some intricacies and it is not ready yet. Paper, however should work just fine (but I have not tested how this behaves on iPad yet).
Also I'm still thinking on prop naming and best way of exposing this functionality, any suggestions would also be welcome. At the moment I'm using dedicated prop temporarily named sheetCustomDetents
which has effect only if:
-
stackPresentation == 'formSheet'
-
sheetAllowedDetents == 'custom'
But I also consider accepting an array with sheetAllowedDetents
prop (so the declaration would be sheetAllowedDetents: SheetDetentTypes | number[]
and dispatching this internally.
I will test it then @kkafar 👍 . For me this one sheetAllowedDetents: SheetDetentTypes | number[]
makes more sense 👍 . Is there a limit of amount of detents do you know? Cause if there is a limit maybe we can be more explicit in the type number[]
.
I'm not aware of any limitation on number of detents. Only restriction I'm aware of right now is that the array has to be sorted in ascending order.
I am testing this and TBH, being able to do things like:
sheetCornerRadius: 20,
sheetAllowedDetents: [0.4, 0.9],
sheetLargestUndimmedDetent: 0,
is just amazing 🔥 .
However:
https://github.com/software-mansion/react-native-screens/issues/1722 This bug is very bad. You do not notice it in a modal
somehow but here is very noticeable see:
https://github.com/software-mansion/react-native-screens/assets/774577/71ea2f1c-387e-4635-a188-77defecb3203
There is an opened PR... but IMO we need to fix this. It kinda breaks the whole experience.
I'm aware of this flickering (on Fabric it gets even worse).
#1767 can't be merged unfortunately, because while it helps with this particular problem, it brings back other already solved issues.
I will see what can be done here.
Will I be able to use a FlatList
or ScrollView
inside the modal with formSheet
presentation?
How can I help launch this? What's left?
Hey @ansh,
This PR is held back by https://github.com/software-mansion/react-native-screens/pull/1974, as I want to have largest common API surface between platforms.
Also I'm still fighting with the "clipping" effect that was reported above. Haven't found a solution without tradeoffs yet (or maybe I just did, as I'm pushing right now).