feat: Add BottomSheetV2 #42201
Closed
feat: Add BottomSheetV2 #42201
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dcalhoun
added
[Type] Code Quality
|
Size Change: -2.13 kB (0%) Total Size: 1.52 MB
ℹ️ View Unchanged
|
dcalhoun
commented
Jul 6, 2022
Comment on lines
-132
to
+121
| style={ { paddingLeft: 0, paddingRight: 0 } } | ||
| style={ { marginBottom: bottomInset + 20 } } |
There was a problem hiding this comment.
The existing BottomSheet includes bottom offset to account for safe area and horizontal padding. I am undecided as to whether this should reside in the BottomSheetV2 abstraction. There are times where the horizontal padding is overridden in certain contexts. Should this be managed by the BottomSheet or the individual context controlled by the child? E.g. PanelBody provides its own padding equaling the padding provided by BottomSheet.
Comment on lines
+43
to
+58
| const { | ||
| animatedHandleHeight, | ||
| animatedSnapPoints, | ||
| animatedContentHeight, | ||
| handleContentLayout, | ||
| } = useBottomSheetDynamicSnapPoints( snapPoints ); |
There was a problem hiding this comment.
It may make more sense to expect a consumer of BottomSheetV2 to control and pass these props to BottomSheetV2. It would likely be a bit verbose, but might offer more flexibility and a simpler BottomSheetV2.
| ref={ ref } | ||
| snapPoints={ animatedSnapPoints } | ||
| > | ||
| <BottomSheetScrollView onLayout={ handleContentLayout }> |
There was a problem hiding this comment.
The current BottomSheet includes a dynamic content container element. It defaults to a ScrollView, but allows switching to a View if the child has navigation. This component will likely need a similar feature long-term.
Comment on lines
102
to
105
| useImperativeHandle( ref, () => ( { | ||
| present: handlePresent, | ||
| dismiss: handleDismiss, | ||
| } ) ); |
There was a problem hiding this comment.
This was originally added while exploring usage for the Editor Help bottom sheet. This code is unused at this point and might should be removed, but was left in place for discussion of an imperative API mirroring the @gorhom/bottom-sheet library.
Comment on lines
+120
to
+136
| <GestureHandlerRootView | ||
| style={ styles[ 'bottom-sheet-v2__gesture-handler' ] } | ||
| > |
There was a problem hiding this comment.
Android modals require a separate GestureHandlerRootView to have dismiss on swipe or backdrop press operate correctly.
| }, [ onClose ] ); | ||
|
|
||
| return ( | ||
| <Modal |
There was a problem hiding this comment.
Modal is used rather than @gorhom/bottom-sheet's BottomSheetModal as the latter is incapable of rendering atop the WP host app header navigation. If the header navigation is ever managed by React Native, then we may be able to switch modal providers and simplify our implementation here.
dcalhoun
added
the
Mobile App - Automation
Sass files ultimately use the StyleSheet API, which is better performance than inline objects.
Remove unnecessary padding removal, ensure proper bottom margin.
Support for backdrop tap and swipe down dimissal requires additional `react-native-gesture-handler` integration. Additionally, Android modal specifically need a special HoC provided by the library.
The dependency absence was oversight.
dcalhoun
force-pushed
the
feat/add-bottom-sheet-v2
branch
from
e9150d2
to
a13fd7b
Compare
Update - Sep 11 2023The failing CI tests appear to be a fairly low-level issue related to This PR targets the Heading level menu via the Support for a However, the aforementioned |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related PRs
What?
Adds
BottomSheetV2as a potential replacement for the existingBottomSheetto address #37559. UpdatesDropdownMenuto utilizeBottomSheetV2.Why?
Relates to #42192. As detailed in #37559, the current
BottomSheetsuffers from a number performance and quality issues.How?
The proposed
BottomSheetV2utilizes@gorhom/bottom-sheetto benefit from the attention to detail placed on features like performance, dynamic height, gestures, etc.Remaining Work
Testing Instructions
Perform the following for the heading level/alignment selection bottom sheets on various devices and orientations.
Screenshots or screencast
Android Screenshots
iOS Screenshots
iOS Interactions
RPReplay_Final1657139083.MP4