-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Unnecessary blank area is created when scrolling down while keyboard is open #19642 #21298
Merged
thienlnam
merged 27 commits into
Expensify:main
from
lukemorawski:lukemorawski19642-blank_area_on_scroll
Nov 19, 2023
Merged
Changes from 22 commits
Commits
Show all changes
27 commits
Select commit
Hold shift + click to select a range
87737bd
viewport scroll block on report screen
lukemorawski 9ef0f83
refactored function declaration
lukemorawski c342cec
formatting, const naming and jsdocs
lukemorawski 6445869
Merge branch 'main' into lukemorawski19642-blank_area_on_scroll
lukemorawski 21fb1b4
Merge branch 'main' into lukemorawski19642-blank_area_on_scroll
lukemorawski 906f744
linting
lukemorawski 0d240cb
even more linting
lukemorawski 75cf222
even more linting
lukemorawski f8c6243
bug fix/firing swipe down call back in SwipeableView on scrolling
lukemorawski 1468987
fix/swipe down on android closes keyboard when scrolling
lukemorawski 279e4c4
swipeableview props desctruct
lukemorawski 4362760
formatting
lukemorawski 6a5e329
Merge branch 'main' into lukemorawski19642-blank_area_on_scroll
lukemorawski bc77e5c
linting
lukemorawski ae79e0a
Merge branch 'main' into lukemorawski19642-blank_area_on_scroll
lukemorawski 8dfc2b6
refactored SwipeableView for browsers to TS
lukemorawski 04af0ba
native swipeable view compatibility with the browser one
lukemorawski f4e948c
Merge branch 'main' into lukemorawski19642-blank_area_on_scroll
lukemorawski fdbfb79
linting
lukemorawski 11bd396
fixes
lukemorawski 4cd8620
Merge branch 'main' into lukemorawski19642-blank_area_on_scroll
lukemorawski 29d12dd
prettier
lukemorawski 7ed0356
added comment to swipeableview style prop
lukemorawski 2a5389a
refactor/nativewebkeyboard to TS
lukemorawski d94490b
Merge branch 'main' into lukemorawski19642-blank_area_on_scroll
lukemorawski b704342
added comments to exported functions on NativeWebKeyboard
lukemorawski 8896a4d
Merge branch 'main' into lukemorawski19642-blank_area_on_scroll
lukemorawski File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,77 @@ | ||
import React, {useEffect, useRef} from 'react'; | ||
import {View} from 'react-native'; | ||
import DomUtils from '@libs/DomUtils'; | ||
import SwipeableViewProps from './types'; | ||
|
||
// Swipeable View is available just on Android/iOS for now. | ||
export default ({children}: SwipeableViewProps) => children; | ||
// Min delta y in px to trigger swipe | ||
const MIN_DELTA_Y = 25; | ||
lukemorawski marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
function SwipeableView({onSwipeUp, onSwipeDown, style, children}: SwipeableViewProps) { | ||
const ref = useRef<View | null>(null); | ||
const scrollableChildRef = useRef<HTMLElement | null>(null); | ||
const startY = useRef(0); | ||
const isScrolling = useRef(false); | ||
|
||
useEffect(() => { | ||
if (!ref.current) { | ||
return; | ||
} | ||
|
||
const element = ref.current as unknown as HTMLElement; | ||
|
||
const handleTouchStart = (event: TouchEvent) => { | ||
startY.current = event.touches[0].clientY; | ||
}; | ||
|
||
const handleTouchEnd = (event: TouchEvent) => { | ||
const deltaY = event.changedTouches[0].clientY - startY.current; | ||
const isSelecting = DomUtils.isActiveTextSelection(); | ||
let canSwipeDown = true; | ||
let canSwipeUp = true; | ||
if (scrollableChildRef.current) { | ||
canSwipeUp = scrollableChildRef.current.scrollHeight - scrollableChildRef.current.scrollTop === scrollableChildRef.current.clientHeight; | ||
canSwipeDown = scrollableChildRef.current.scrollTop === 0; | ||
} | ||
|
||
if (deltaY > MIN_DELTA_Y && onSwipeDown && !isSelecting && canSwipeDown && !isScrolling.current) { | ||
onSwipeDown(); | ||
} | ||
|
||
if (deltaY < -MIN_DELTA_Y && onSwipeUp && !isSelecting && canSwipeUp && !isScrolling.current) { | ||
onSwipeUp(); | ||
} | ||
isScrolling.current = false; | ||
}; | ||
|
||
const handleScroll = (event: Event) => { | ||
isScrolling.current = true; | ||
if (!event.target || scrollableChildRef.current) { | ||
return; | ||
} | ||
scrollableChildRef.current = event.target as HTMLElement; | ||
}; | ||
|
||
element.addEventListener('touchstart', handleTouchStart); | ||
element.addEventListener('touchend', handleTouchEnd); | ||
element.addEventListener('scroll', handleScroll, true); | ||
|
||
return () => { | ||
element.removeEventListener('touchstart', handleTouchStart); | ||
element.removeEventListener('touchend', handleTouchEnd); | ||
element.removeEventListener('scroll', handleScroll); | ||
}; | ||
}, [onSwipeDown, onSwipeUp]); | ||
|
||
return ( | ||
<View | ||
ref={ref} | ||
style={style} | ||
> | ||
{children} | ||
</View> | ||
); | ||
} | ||
|
||
SwipeableView.displayName = 'SwipeableView'; | ||
|
||
export default SwipeableView; |
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,18 @@ | ||
import {ReactNode} from 'react'; | ||
import {StyleProp, ViewStyle} from 'react-native'; | ||
|
||
type SwipeableViewProps = { | ||
/** The content to be rendered within the SwipeableView */ | ||
children: ReactNode; | ||
|
||
/** Callback to fire when the user swipes down on the child content */ | ||
onSwipeDown: () => void; | ||
onSwipeDown?: () => void; | ||
|
||
/** Callback to fire when the user swipes up on the child content */ | ||
onSwipeUp?: () => void; | ||
|
||
/** Style for the wrapper View */ | ||
style?: StyleProp<ViewStyle>; | ||
}; | ||
|
||
export default SwipeableViewProps; |
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
/** | ||
* A hook that blocks viewport scroll when the keyboard is visible. | ||
* It does this by capturing the current scrollY position when the keyboard is shown, then scrolls back to this position smoothly on 'touchend' event. | ||
* This scroll blocking is removed when the keyboard hides. | ||
* This hook is doing nothing on native platforms. | ||
* | ||
* @example | ||
* useBlockViewportScroll(); | ||
*/ | ||
function useBlockViewportScroll() { | ||
// This hook is doing nothing on native platforms. | ||
// Check index.ts for web implementation. | ||
} | ||
|
||
export default useBlockViewportScroll; |
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,43 @@ | ||
import {useEffect, useRef} from 'react'; | ||
import Keyboard from '@libs/NativeWebKeyboard'; | ||
|
||
/** | ||
* A hook that blocks viewport scroll when the keyboard is visible. | ||
* It does this by capturing the current scrollY position when the keyboard is shown, then scrolls back to this position smoothly on 'touchend' event. | ||
* This scroll blocking is removed when the keyboard hides. | ||
* This hook is doing nothing on native platforms. | ||
* | ||
* @example | ||
* useBlockViewportScroll(); | ||
*/ | ||
function useBlockViewportScroll() { | ||
const optimalScrollY = useRef(0); | ||
const keyboardShowListenerRef = useRef(() => {}); | ||
const keyboardHideListenerRef = useRef(() => {}); | ||
|
||
useEffect(() => { | ||
const handleTouchEnd = () => { | ||
window.scrollTo({top: optimalScrollY.current, behavior: 'smooth'}); | ||
}; | ||
|
||
const handleKeybShow = () => { | ||
optimalScrollY.current = window.scrollY; | ||
window.addEventListener('touchend', handleTouchEnd); | ||
}; | ||
|
||
const handleKeybHide = () => { | ||
window.removeEventListener('touchend', handleTouchEnd); | ||
}; | ||
|
||
keyboardShowListenerRef.current = Keyboard.addListener('keyboardDidShow', handleKeybShow); | ||
keyboardHideListenerRef.current = Keyboard.addListener('keyboardDidHide', handleKeybHide); | ||
|
||
return () => { | ||
keyboardShowListenerRef.current(); | ||
keyboardHideListenerRef.current(); | ||
window.removeEventListener('touchend', handleTouchEnd); | ||
}; | ||
}, []); | ||
} | ||
|
||
export default useBlockViewportScroll; |
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
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
Oops, something went wrong.
Oops, something went wrong.
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.
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.
We should apply the style prop here as well same as web.
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.
great spotting! But this is deliberate :) When the style is added in native it brakes the layout
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.
Ah, It will make this component inconsistent across platforms. May be we can manage the styles in such a way that works across platforms.
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.
Otherwise, Add a comment here to explain why styles are not applied. I would still prefer it be consistent.
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.
left the comment as I don't really have the capacity now to deal with that styling problem. Hopefully that's good enough.