react-native-screens icon indicating copy to clipboard operation
react-native-screens copied to clipboard

feat(iOS): support for custom detents with `formSheet` presentation

Open kkafar opened this issue 1 year ago • 9 comments

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

kkafar avatar Aug 01 '23 13:08 kkafar

Do you want some help to test this @kkafar ? Also, if you make this native for Android... you will be a hero 😅.

ferrannp avatar Aug 02 '23 14:08 ferrannp

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:

  1. stackPresentation == 'formSheet'
  2. sheetAllowedDetents == 'custom'

But I also consider accepting an array with sheetAllowedDetents prop (so the declaration would be sheetAllowedDetents: SheetDetentTypes | number[] and dispatching this internally.

kkafar avatar Aug 03 '23 06:08 kkafar

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[].

ferrannp avatar Aug 04 '23 11:08 ferrannp

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.

kkafar avatar Aug 08 '23 09:08 kkafar

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.

ferrannp avatar Aug 10 '23 21:08 ferrannp

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.

kkafar avatar Aug 11 '23 05:08 kkafar

Will I be able to use a FlatList or ScrollView inside the modal with formSheet presentation?

ansh avatar Oct 18 '23 19:10 ansh

How can I help launch this? What's left?

ansh avatar Dec 13 '23 11:12 ansh

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

kkafar avatar Dec 13 '23 11:12 kkafar