-
Notifications
You must be signed in to change notification settings - Fork 30
feat(components-native): Replace react-native-modalize with react-native-bottom-sheet for ContentOverlay #2819
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: master
Are you sure you want to change the base?
feat(components-native): Replace react-native-modalize with react-native-bottom-sheet for ContentOverlay #2819
Conversation
…ttom sheet and keyboard positioning
…rop in BottomSheet
|
|
||
| const draggable = onBeforeExit ? false : isDraggable; | ||
| // Prevent the Overlay from being flush with the top of the screen, even if we are "100%" or "fullscreen" | ||
| const topInset = insets.top || tokens["space-larger"]; |
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.
on Android the top inset is coming back as 0, resulting in the overlay being flush against the top bar of the device
it seems we always want a gap, even if it's "fullscreen" and while this works for iOS we need to provide a value for Android.
the alternative is to set snap points that are NOT 100%, which would do the same thing but it makes the code a little more complicated since iOS would have both the insets and the reduced snap point.
we could remove the insets altogether if we want, and strictly use snappoints... though that will likely get interesting with the dynamic sizing because we don't use snap points there so this might be the best given our goal.
|
Published Pre-release for 115d12f with versions: To install the new version(s) for Mobile run: |
remove erroneous documenation
* deprecate legacy prop * update props * fully removed unused props
* uninstall modalize * sync lockfile with npm i * remove Modalize reference in override
* add ContentOverlayProvider, integrate with InputText to handle keyboard interactions * avoid TS error, fix mock, add keyboard config props * remove redundant test * use interactive to prevent issues when no snap points * prevent drag/pan in new way, always provide snap points to fix keyboard dismiss restoration * add context for our single snap point entry * use simpler callback for onClose * add a couple tests * handle large screen modal flow * remove unnecessary ContentOverlay useEffect * update draggable logic and comments * replace provider in tests * remove low value tests
| export default meta; | ||
| type Story = StoryObj<typeof meta>; | ||
|
|
||
| const BasicTemplate = () => { |
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.
| // While we no longer need to adhere to this somewhat awkward behavior, we will leave it as is until a larger refactor. | ||
| if (onBeforeExit) { | ||
| return false; | ||
| } |
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.
Throwing this comment here as it's related.
When I supply onBeforeExit, the new ContentOverlay is not fullscreen and also is not dismissible at all (no dismiss button, no swipe down, no clickaway handling). I'm unsure whether the lack of fullscreen is a problem, but the lack of dismiss functionality seems odd. Actually, adding the fullScreen prop solves both of the problems above. But even without fullScreen, I'm pretty sure we might expect it to be at least dismissible with swipe down.
Is that expected or a bug?
Screen.Recording.2026-01-19.at.12.47.21.PM.mov
Here's my test case, note that InnerContent is just a few Text nodes.
<ContentOverlay
ref={overlayRef}
title="Welcome NEW!"
onBeforeExit={() => {
overlayRef.current?.close?.();
}}
>
<InnerContent />
</ContentOverlay>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.
good catch. one of those issues is from the draggable logic for sure, I can address that one pretty easily.
aside from that though, there's a whole mess to untangle here. I'm going to take a swing at making a testable module and ideally a small table/chart because there are too many permutations and weird combinations that I'm not entirely sure what is expected in each case.
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 know you already know this, but just following up here for posterity - I have a separate PR to address this: #2884
since it was originally built against the legacy ContentOverlay, it is the blueprint for the logic/behavior we expect.
it should be approximately equivalent now with the new library consuming and implementing those abstracted behavior descriptions - though there are a couple things that might behave a little bit different. I know there are at least 2 bugs in the old version.
the deprecated version of the component uses both showDismiss and shouldShowDismiss allowing related code & functionality to get out of sync
and there was a strange pan gesture interaction I encountered with the default props but I can't seem to reproduce it now. maybe it was only happening on Android.
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.
hmmm it's still misbehaving. I'll have to fix that.
I think I know what the problem is now though at least!
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.
ok I get it now. the old one has a bunch of really invalid combinations.
onBeforeExit + adjustToContentHeight means you cannot close it at all
you must explicitly set showDismiss={true} because otherwise it will fail all the other conditions for shouldShowDismiss and you now have no way to dismiss the overlay since both dragging and background presses have been disabled.
this component's props API is a nightmare. I guess I will reproduce the existing behavior to avoid making breaking changes but none of this makes any sense.
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 also don't like the old behaviour.. so if it's worth breaking this logic here to make an improvement and then double checking all existing usages are fine (maybe during the integration ticket), then I'm fine that!
Also fine with matching the old behaviour even though it doesn't make sense. I think we both know how much this component's API needs to be adjusted... so that sounds like a future ticket.
EDIT: I see you made the change in the other PR 👍
| updateFormAndState(removedIOSCharValue); | ||
| } | ||
|
|
||
| function handleBottomSheetFocus(event?: FocusEvent) { |
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 like Android's keyboard handling is behaving a bit weird.. shows a large blank space that takes up the scroll view container. Not sure if this is related to focus/blur handling here, or something configurable on the BottomSheetScrollView itself.
Screen.Recording.2026-01-19.at.1.58.41.PM.mov
Here's the code I used for testing... just throw some Texts and inputs in there for testing.
<ContentOverlay
title="Welcome NEW!"
scrollEnabled
>
<InnerContent />
</ContentOverlay>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.
that was an issue I had at one point, however from my testing I have not seen it since I applied a fix so that's odd it's happening again for you.
would you mind sharing the specs of the emulator you're running it on?
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.
DM'd but also posting here, my emulator is Pixel_3a_API_34_extension_level_7_arm64-v8
| })); | ||
| } | ||
|
|
||
| function handleBottomSheetBlur(event?: FocusEvent) { |
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.
Throwing this comment here, though not sure if it's related to this code.
Seeing some weird behaviour on a physical iOS device.. however it's an iPod Touch and on a really old iOS 15. So I'm not sure if we need to worry about this case, unless someone can repro on a more modern device.
When dismissing the keyboard with Done on the keyboard, it mostly seems to work fine. However, when dismissing the keyboard by tapping the ContentOverlay, the keyboard hides and leaves a blank white container taking up space.
ipod.ios.15.MP4
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.
same as the other weird blank space keyboard thing you mentioned, I also saw this at one point but not since some fixes were applied.
notably this does happen on the OLD ContentOverlay with certain props. is there any chance this is rendering an instance of that?
if not, I'll have to try some other devices/iOS versions because neither my physical iOS device nor my simulators are exhibiting that behavior.
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.
notably this does happen on the OLD ContentOverlay with certain props. is there any chance this is rendering an instance of that?
nope, definitely the new ContentOverlay! I DM'd you my QA branch for JM.
If we can't repro this on something more modern, we can resolve this thread 👍
…rity (#2884) * add behavior computation module with tests, implement in ContentOverlay * clean up tests * update spelling for consistency * update logic to mirror existing * revert back to position for top logic
| {renderHeader()} | ||
| <View testID="ATL-Overlay-Children">{children}</View> | ||
| </BottomSheetScrollView> | ||
| <BottomSheetView style={{ height: windowHeight }}> |
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.
Wow, that was it? Nice!
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 hoping so! it seems to have fixed it for me, fingers crossed that holds true.
I found this solution and one other one where someone was saying that a lack of a fixed height dimension on the parent was the issue. their fix was to apply a height to the entire app, but I was curious if you could do it for the scrollview parent instead.
…oller (#2889) * add RNKC, use to fix SV issues in ContentOverlay * prevent ts errors in mocks * provide override for RNKC package * add additional mocks for site * add aliases for RN packages to avoid vite failures * revert aliases, use exernal instead * surgically correct package-lock state * update package-lock dep locations
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.
Approved 🎉
Refreshed the Blocked label as we're not planning on merging this until we do the integration work in JM.

Remaining tasks
keyboardAvoidingBehaviorprop not implementedavoidKeyboardLikeIOSprop not implementedmodalForLargeScreensstyle not appliedBottomSheetModalandBottomSheetScrollViewinMockBottomSheetContentOverlay, there should be no rebuilt.We also need to go through existing ContentOverlay usages and find out what kind of inputs are currently being used. I realized
BottomSheet.InputTextmay not be the only wrapper input component we need. To recap,BottomSheet.InputTextis a wrapper aroundInputTextthat implementsonFocusandonBlurto properly position keyboard against these inputs. If we do indeed need more wrapper inputs, we will need to follow the pattern inBottomSheet.InputTextfor every input component, and replace each existing instance 🥲This PR targets #2803, but does not depend on it.
Motivations
Changes
Added
Changed
Deprecated
Removed
Fixed
Security
Testing
ContentOverlay.rebuilt.tsxis created for the sake of testing the old version and compare against the new one in simulator. This should not be a rebuilt and should completely replaceContentOverlay.tsxTo test with live linking:
meta.jsonChanges can be
tested via Pre-release
In Atlantis we use Github's built in pull request reviews.