-
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
26401 avoid scroll on virtual viewport #35256
26401 avoid scroll on virtual viewport #35256
Conversation
Hi @fedirjh I have fix code lint, prettier, and this PR is ready to review |
@suneox Sure, reviewing shortly. |
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.
Let's fix this bug in other places too:
- Profile display name
- Add new contact method
- Workspace settings -> name
- Private notes: nagivate to report -> click on report header
- Request money flow: there are several pages (waypoint selection, request flow, set merchant, description..)
- Task Flow (create, task title edit, task description edit)
@suneox What would happen if we enable shouldAwareViewportScroll
by default ? will this affect scrollable views?
src/hooks/useIsInputFocus/index.ts
Outdated
import {useCallback, useEffect} from 'react'; | ||
import useDebouncedState from '@hooks/useDebouncedState'; | ||
|
||
export default function useIsInputFocus(enable = false): boolean { |
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.
Let's add a comment explaining this hook and what it does.
src/hooks/useIsInputFocus/index.ts
Outdated
import {useCallback, useEffect} from 'react'; | ||
import useDebouncedState from '@hooks/useDebouncedState'; | ||
|
||
export default function useIsInputFocus(enable = false): boolean { |
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.
export default function useIsInputFocus(enable = false): boolean { | |
export default function useTackInputFocus(enable = false): boolean { |
let's rename it to a more verbose name
src/components/ScreenWrapper.tsx
Outdated
const isInputFocus = useIsInputFocus(shouldAwareViewportScroll && Browser.isMobileSafari()); | ||
const isAwareViewportScroll = shouldEnableMaxHeight && isInputFocus; |
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.
const isInputFocus = useIsInputFocus(shouldAwareViewportScroll && Browser.isMobileSafari()); | |
const isAwareViewportScroll = shouldEnableMaxHeight && isInputFocus; | |
const isAwareViewportScroll = useTackInputFocus(shouldEnableMaxHeight && shouldAwareViewportScroll && Browser.isMobileSafari()); |
This could be simplified.
Hi @fedirjh,
and "..." is meaning for how many page?
I'm not sure when we enable I have checked the list page you have provided above almost work well. Excludes
What do you think if we separate the scope for this ticket? |
@suneox We already implemented the main fix within the CleanShot.2024-01-31.at.18.09.29.mp4CleanShot.2024-01-31.at.18.14.56.mp4
The flow has create and edit forms; affected forms are related to merchant description and waypoint point selector, I have included that in the video.
Sounds good.
I have included all affected pages in the video
We are fixing the same bug, and I think we will only apply the new prop; no other changes are required IMO, If any page requires extra changes, then we will handle it on a separate ticket. |
@fedirjh Thank you for your feedback, I'll update the list page you've provided asap |
@fedirjh I have updated your feedback and we have a condition |
cc @suneox could you please merge main? Also we have some failing tests, could you please fix them? |
…-virtual-viewport
@fedirjh I have syned with latest main |
@fedirjh Have you got any for this PR? |
Testing it shortly. |
@suneox It seems that the hook used has some delay in it. So the bug is still reproducible with quicker steps :
CleanShot.2024-02-06.at.00.51.11.mp4 |
Let me double-check |
@fedirjh I have handled the edge case, quickly scrolling after focusing to input Screen.Recording.2024-02-06.at.23.13.52.mov |
Reviewer Checklist
Screenshots/VideosAndroid: NativeCleanShot.2024-02-06.at.19.04.54.mp4Android: mWeb ChromeCleanShot.2024-02-06.at.19.06.18.mp4iOS: NativeCleanShot.2024-02-06.at.18.52.42.mp4iOS: mWeb SafariCleanShot.2024-02-06.at.18.32.37.mp4MacOS: Chrome / SafariCleanShot.2024-02-06.at.18.48.58.mp4MacOS: DesktopCleanShot.2024-02-06.at.19.08.32.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.
Looks good.
src/hooks/useTackInputFocus/index.ts
Outdated
import useDebouncedState from '@hooks/useDebouncedState'; | ||
|
||
/** | ||
* This hook to detech input or text area focus on browser, to avoid scroll on vitual viewport |
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.
* This hook to detech input or text area focus on browser, to avoid scroll on vitual viewport | |
* This hook to detect input or text area focus on browser, to avoid scroll on virtual viewport |
little typo.
src/components/ScreenWrapper.tsx
Outdated
@@ -220,12 +227,12 @@ function ScreenWrapper( | |||
{...keyboardDissmissPanResponder.panHandlers} | |||
> | |||
<KeyboardAvoidingView | |||
style={[styles.w100, styles.h100, {maxHeight}]} | |||
style={[styles.w100, styles.h100, {maxHeight}, isAwareViewportScroll && [styles.overflowAuto, styles.overscrollBehaviorContain]]} |
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.
style={[styles.w100, styles.h100, {maxHeight}, isAwareViewportScroll && [styles.overflowAuto, styles.overscrollBehaviorContain]]} | |
style={[styles.w100, styles.h100, {maxHeight}, isAwareViewportScroll ? [styles.overflowAuto, styles.overscrollBehaviorContain] : {}]} |
To avoid applying false
to style.
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 good to me.
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 good. Just some name changes
src/components/ScreenWrapper.tsx
Outdated
@@ -79,6 +80,9 @@ type ScreenWrapperProps = { | |||
/** Whether to show offline indicator */ | |||
shouldShowOfflineIndicator?: boolean; | |||
|
|||
/** Whether to avoid scroll on virtual viewport */ | |||
shouldAwareViewportScroll?: boolean; |
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.
shouldAwareViewportScroll?: boolean; | |
shouldAvoidScrollOnVirtualViewport?: boolean; |
src/components/ScreenWrapper.tsx
Outdated
@@ -109,6 +113,7 @@ function ScreenWrapper( | |||
onEntryTransitionEnd, | |||
testID, | |||
navigation: navigationProp, | |||
shouldAwareViewportScroll = true, |
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.
shouldAwareViewportScroll = true, | |
shouldAvoidScrollOnVirtualViewport = true, |
src/components/ScreenWrapper.tsx
Outdated
@@ -192,6 +197,8 @@ function ScreenWrapper( | |||
// eslint-disable-next-line react-hooks/exhaustive-deps | |||
}, []); | |||
|
|||
const isAwareViewportScroll = useTackInputFocus(shouldEnableMaxHeight && shouldAwareViewportScroll && Browser.isMobileSafari()); |
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.
const isAwareViewportScroll = useTackInputFocus(shouldEnableMaxHeight && shouldAwareViewportScroll && Browser.isMobileSafari()); | |
const isAvoidingViewportScroll = useTackInputFocus(shouldEnableMaxHeight && shouldAvoidScrollOnVirtualViewport && Browser.isMobileSafari()); |
src/components/ScreenWrapper.tsx
Outdated
@@ -220,12 +227,12 @@ function ScreenWrapper( | |||
{...keyboardDissmissPanResponder.panHandlers} | |||
> | |||
<KeyboardAvoidingView | |||
style={[styles.w100, styles.h100, {maxHeight}]} | |||
style={[styles.w100, styles.h100, {maxHeight}, isAwareViewportScroll ? [styles.overflowAuto, styles.overscrollBehaviorContain] : {}]} |
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.
style={[styles.w100, styles.h100, {maxHeight}, isAwareViewportScroll ? [styles.overflowAuto, styles.overscrollBehaviorContain] : {}]} | |
style={[styles.w100, styles.h100, {maxHeight}, isAvoidingViewportScroll ? [styles.overflowAuto, styles.overscrollBehaviorContain] : {}]} |
src/components/ScreenWrapper.tsx
Outdated
behavior={keyboardAvoidingViewBehavior} | ||
enabled={shouldEnableKeyboardAvoidingView} | ||
> | ||
<PickerAvoidingView | ||
style={styles.flex1} | ||
style={isAwareViewportScroll ? [styles.h100, {marginTop: 1}] : styles.flex1} |
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.
style={isAwareViewportScroll ? [styles.h100, {marginTop: 1}] : styles.flex1} | |
style={isAvoidingViewportScroll ? [styles.h100, {marginTop: 1}] : styles.flex1} |
@@ -0,0 +1,6 @@ | |||
/** | |||
* This hook to detect input or text area focus on browser, on native doesn't support DOM so default 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.
* This hook to detect input or text area focus on browser, on native doesn't support DOM so default return false | |
* Detects input or text area focus on browsers. Native doesn't support DOM so default to false |
src/hooks/useTackInputFocus/index.ts
Outdated
import useDebouncedState from '@hooks/useDebouncedState'; | ||
|
||
/** | ||
* This hook to detect input or text area focus on browser, to avoid scroll on virtual viewport |
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.
* This hook to detect input or text area focus on browser, to avoid scroll on virtual viewport | |
* Detects input or text area focus on browsers to avoid scrolling on virtual viewports |
* This hook to detect input or text area focus on browser, to avoid scroll on virtual viewport | ||
*/ | ||
export default function useTackInputFocus(enable = false): boolean { | ||
const [, isInputFocusDebounced, setIsInputFocus] = useDebouncedState(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.
Can we define the first value? Or does that throw lint errors?
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.
hi @luacmartins I have updated your suggestions and when we define the first value but not using that will throw eslint error.
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, let's keep it empty then
@suneox can you please merge main? Maybe that will fix the failing test |
…-virtual-viewport
Hi @luacmartins the pipeline has fixed |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/luacmartins in version: 1.4.39-0 🚀
|
🚀 Deployed to production by https://github.com/Beamanator in version: 1.4.39-8 🚀
|
Details
Fixed Issues
$ #26401
PROPOSAL: #26401 (comment)
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Screen.Recording.2024-01-27.at.00.14.39.mov
Android: mWeb Chrome
Screen.Recording.2024-01-27.at.00.13.30.mov
iOS: Native
Screen.Recording.2024-01-26.at.18.16.46.mov
iOS: mWeb Safari
Screen.Recording.2024-01-26.at.17.14.27.mov
MacOS: Chrome / Safari
Screen.Recording.2024-01-26.at.17.15.44.mov
MacOS: Desktop
Screen.Recording.2024-01-27.at.00.05.56.mov