[RNMobile] Bottom-sheet: Add custom header #30291
Merged
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
fluiddot
added
[Type] Bug
|
Size Change: 0 B Total Size: 1.31 MB ℹ️ View Unchanged
|
42 tasks
fluiddot
commented
Apr 30, 2021
Comment on lines
-107
to
+106
| ( safeAreaBottomInset || | ||
| styles.list.paddingBottom ) + searchFormHeight, | ||
| safeAreaBottomInset || styles.list.paddingBottom, |
There was a problem hiding this comment.
We no longer need to add the search form height here because this value is now included in the calculations of the maxHeight, which is part of the style prop contentContainerStyle.
fluiddot
commented
Apr 30, 2021
Comment on lines
+100
to
+102
| if ( ! this.props.isVisible ) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
I decided to prevent executing any logic related to keyboard show/hide events for non-visible bottom-sheets. Not sure if there would be a case that would require this but at least we should prevent triggering layout animations because they're global.
This value that was being used for calculating the padding bottom of the search results view is no longer needed.
fluiddot
commented
Apr 30, 2021
Comment on lines
113
to
177
| this.keyboardWillShowListener = Keyboard.addListener( | ||
| 'keyboardWillShow', | ||
| this.keyboardWillShow | ||
| this.keyboardShowListener = Keyboard.addListener( | ||
| Platform.OS === 'ios' ? 'keyboardWillShow' : 'keyboardDidShow', | ||
| this.keyboardShow | ||
| ); | ||
|
|
||
| this.keyboardDidHideListener = Keyboard.addListener( | ||
| 'keyboardDidHide', | ||
| this.keyboardDidHide | ||
| this.keyboardHideListener = Keyboard.addListener( | ||
| Platform.OS === 'ios' ? 'keyboardWillHide' : 'keyboardDidHide', | ||
| this.keyboardHide |
There was a problem hiding this comment.
Will keyboard events are not available on Android (documentation reference).
There was a problem hiding this comment.
It may be worthwhile leaving a code comment inline stating this.
There was a problem hiding this comment.
Good idea! I've added the comment in this commit.
fluiddot
force-pushed
the
rnmobile/add/custom-header-bottom-sheet
branch
from
d30efdb to
e35bff5
Compare
fluiddot
commented
Apr 30, 2021
| this.keyboardHeight = height; | ||
| this.performKeyboardLayoutAnimation( e ); | ||
| this.onSetMaxHeight(); | ||
| this.props.onKeyboardShow?.(); |
There was a problem hiding this comment.
This callback and onKeyboardHide will allow us to sync potential changes in the layout with the layout animations triggered from performKeyboardLayoutAnimation.
dcalhoun
approved these changes
May 3, 2021
There was a problem hiding this comment.
LGTM. Tested on an iPhone SE and Samsung Galaxy S20.
The supplied patch did not work for me as changes were made after the patch creation. So, I created a new patch below if anyone needs it for future use.
Add header to inserter diff
diff --cc packages/block-editor/src/components/inserter/menu.native.js
index a81cfbd184,3032d0b39d..0000000000
--- a/packages/block-editor/src/components/inserter/menu.native.js
+++ b/packages/block-editor/src/components/inserter/menu.native.js
@@@ -39,8 -39,10 +39,9 @@@ function InserterMenu(
insertionIndex,
} ) {
const [ filterValue, setFilterValue ] = useState( '' );
- const [ searchFormHeight, setSearchFormHeight ] = useState( 0 );
// eslint-disable-next-line no-undef
const [ showSearchForm, setShowSearchForm ] = useState( __DEV__ );
+ const [ showExtraHeader, setShowExtraHeader ] = useState( true );
const {
showInsertionPoint,
@@@ -180,14 -182,33 +181,29 @@@
<BottomSheet
isVisible={ true }
onClose={ onClose }
+ onKeyboardShow={ () => setShowExtraHeader( false ) }
+ onKeyboardHide={ () => setShowExtraHeader( true ) }
header={
showSearchForm && (
- <InserterSearchForm
- onChange={ ( value ) => {
- setFilterValue( value );
- } }
- value={ filterValue }
- />
+ <>
+ <InserterSearchForm
+ onChange={ ( value ) => {
+ setFilterValue( value );
+ } }
+ value={ filterValue }
- onLayout={ ( event ) => {
- const { height } = event.nativeEvent.layout;
- setSearchFormHeight( height );
- } }
+ />
+ { showExtraHeader && (
+ <View
+ style={ {
+ marginHorizontal: 8,
+ marginBottom: 20,
+ height: 40,
+ borderRadius: 8,
+ backgroundColor: 'black',
+ } }
+ />
+ ) }
+ </>
)
}
hasNavigation
Comment on lines
125
to
145
| if ( duration && easing ) { | ||
| const animationConfig = { | ||
| // We have to pass the duration equal to minimal accepted duration defined here: RCTLayoutAnimation.m | ||
| duration: duration > 10 ? duration : 10, | ||
| type: LayoutAnimation.Types[ easing ] || 'keyboard', | ||
| }; | ||
| const layoutAnimation = { | ||
| duration: animationConfig.duration, | ||
| update: animationConfig, | ||
| create: { | ||
| ...animationConfig, | ||
| property: LayoutAnimation.Properties.opacity, | ||
| }, | ||
| delete: { | ||
| ...animationConfig, | ||
| property: LayoutAnimation.Properties.opacity, | ||
| }, | ||
| }; | ||
| LayoutAnimation.configureNext( layoutAnimation ); | ||
| this.lastLayoutAnimation = layoutAnimation; | ||
| } else { |
There was a problem hiding this comment.
I thought the original note in your other branch that this code came form React Native's internal KeyboardAvoidingView was helpful, personally. It may be worth adding the comment that is the origin of this code.
There was a problem hiding this comment.
I decided to remove it because I wasn't really applying the same code but I agree that it would be helpful to keep the reference 👍 , I reverted the change in this commit.
Comment on lines
113
to
177
| this.keyboardWillShowListener = Keyboard.addListener( | ||
| 'keyboardWillShow', | ||
| this.keyboardWillShow | ||
| this.keyboardShowListener = Keyboard.addListener( | ||
| Platform.OS === 'ios' ? 'keyboardWillShow' : 'keyboardDidShow', | ||
| this.keyboardShow | ||
| ); | ||
|
|
||
| this.keyboardDidHideListener = Keyboard.addListener( | ||
| 'keyboardDidHide', | ||
| this.keyboardDidHide | ||
| this.keyboardHideListener = Keyboard.addListener( | ||
| Platform.OS === 'ios' ? 'keyboardWillHide' : 'keyboardDidHide', | ||
| this.keyboardHide |
There was a problem hiding this comment.
It may be worthwhile leaving a code comment inline stating this.
|
@dcalhoun Heads up that I introduced a fix for preventing running multiple layout animations on Android (commit), I'd appreciate it if you could do a second review, thanks 🙇 ! This fix is related to the erroneous opacity issue we're discussing in #31151 (comment). |
Yep, sorry for that, and thanks for providing a new version 🙇 . I've updated the patch diff in the PR's description with a newer version due to the changes from recent commits. |
dcalhoun
approved these changes
May 6, 2021
There was a problem hiding this comment.
Latest changes LGTM. I was unable to test on my iPhone SE due to builds issues unrelated to this PR. I tested on a Samsung Galaxy S20.
Android still has some animation jumping (matches current video in PR description), but my understanding is that is the current target. I did not experience any issues on Android regarding to the opacity of the block types within the inserter. 🎉
Thank you very much @dcalhoun for the review ❤️ !
Yeah, the jump that happens right after the keyboard is shown/hide is caused by the lack of the |
4 tasks
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.
Description
Add a
headerprop for rendering a custom header in the bottom-sheet component. This PR also introduces fixes to address the issue related to the bottom-sheet component and the keyboard show/hide animation.Additionally, it also modifies how the inserter search form is rendered in the inserter menu, now it's rendered via the new
headerprop.How has this been tested?
Check header changes upon keyboard show/hide
As an additional test, I applied the changes from the patch diff to verify if making changes in the header when the keyboard is shown/hide could produce visual glitches.
Patch diff
Screenshots
bottom-sheet-header-ios.mp4
bottom-sheet-header-extra-ios.mp4
bottom-sheet-header-android.mp4
bottom-sheet-header-extra-android.mp4
As you can see on the videos, on Android the animation is different because we can't listen to the
willevents of the keyboard.Types of changes
Bug fix
Checklist:
*.native.jsfiles for terms that need renaming or removal).