feat(components-native): Replace react-native-modalize with react-native-bottom-sheet for ContentOverlay #2819
Conversation
…e-react-native-modalize' of github.com:GetJobber/atlantis into JOB-140606-implement-it-in-content-overlay
Deploying atlantis with
|
| Latest commit: |
2e3c550
|
| Status: | 🚫 Build failed. |
There was a problem hiding this comment.
This is needed to handle dismissing ContentOverlay on Android hardware back button press.
| const isFullScreenOrTopPosition = | ||
| fullScreen || (!adjustToContentHeight && currentIndex === 1); | ||
| const shouldShowDismiss = | ||
| showDismiss || isScreenReaderEnabled || isFullScreenOrTopPosition; |
There was a problem hiding this comment.
There is still some issues with this. In react-native-modalize there is a onPositionChange callback where position is either initial or top. When the overlay content does not take up the full height and is draggable, the position starts at initial. When you drag up to open to the 100% snap point, the position is top.
However, in @gorhom/bottom-sheet, where you can get the "position" of the bottom sheet via onChange: (index: number) => void, and index is one of -1(closed), 0(open at initial snap point, mostly likely at content height), and 1 (at 100% snap point), index is only 1 at the top when you have two or more snap points. This means if you have full screen, or a lot of content that you set scrollEnabled and the overlay is opening up at 100% right away, the index of that is 0, not 1.
There was a problem hiding this comment.
A potential solution is, onChange actually passes back position: number as well even though it's not documented. I think that's the position of the sheet in px starting at the top.
Actually, I'm just gonna try that.
There was a problem hiding this comment.
Turns out that works. position is 0 when the overlay is at the top. It excludes the top inset.
| const snapPoints = useMemo(() => ["100%"], []); | ||
|
|
||
| useImperativeHandle(ref, () => ({ | ||
| open: () => { |
There was a problem hiding this comment.
Map to the existing API to open and close
|
|
||
| // If isDraggable is true, we always want to have a snap point at 100% | ||
| // enableDynamicSizing will add another snap point of the content height | ||
| const snapPoints = useMemo(() => ["100%"], []); |
There was a problem hiding this comment.
The old implementation had a lot of logic here, but it's significantly simplified here due to how the library handles snap points. From what I can tell, we want to always provide the ability to snap to 100% when isDraggable is true.
So we will always have 100% in snapPoints.
When enableDynamicSizing is true, it automatically adds another snap point of the content height to snapPoints. For example, [1000] becomes [400, 1000].
| const reactTag = findNodeHandle(overlayHeader.current); | ||
|
|
||
| if (reactTag) { | ||
| AccessibilityInfo.setAccessibilityFocus(reactTag); |
There was a problem hiding this comment.
I haven't actually had the chance to test if this is working the way we intend it to. This was brought over from the existing implementation
| // enableDynamicSizing will add another snap point of the content height | ||
| const snapPoints = useMemo(() => ["100%"], []); | ||
|
|
||
| const onCloseController = () => { |
There was a problem hiding this comment.
Existing implementations of onBeforeExit is used for things like resetting forms that are in the overlay when you leave.
When onBeforeExit is supplied:
- Click on backdrop to close is disabled
- Pan down to close is disabled
You would have to exit through the dismiss button, or the Android back button.
| backdropComponent={props => ( | ||
| <Backdrop {...props} pressBehavior={onBeforeExit ? "none" : "close"} /> | ||
| )} | ||
| name="content-overlay-rebuilt" |
There was a problem hiding this comment.
Official documentation states that the name prop is "Modal name to help identify the modal for later on." I am not sure how it could be used or if it even is applicable to how we use ContentOverlay
| enablePanDownToClose={draggable} | ||
| enableContentPanningGesture={draggable} | ||
| enableDynamicSizing={!fullScreen} | ||
| topInset={insets.top} |
There was a problem hiding this comment.
This avoids the bottom sheet modal to open all the way up into the top inset area. It looks different from the existing implementation:
With @gorhom/bottom-sheet:
…e-react-native-modalize' into JOB-140606-implement-it-in-content-overlay
…e-react-native-modalize' into JOB-140606-implement-it-in-content-overlay
…e-react-native-modalize' into JOB-140606-implement-it-in-content-overlay
| import type { MissingTranslationError } from "react-intl"; | ||
| import * as ReactNative from "react-native"; | ||
| import React from "react"; | ||
| import type { Modalize, ModalizeProps } from "react-native-modalize"; |
There was a problem hiding this comment.
Started ripping out react-native-modalize.
TODO: finish ripping it out
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.