-
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
Enhance Carousel Scrolling and Interaction in AttachmentCarousel #39930
Merged
deetergp
merged 6 commits into
Expensify:main
from
kidroca:kidroca/fix/carousel-opens-on-wrong-attachment
May 28, 2024
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
0887405
🛠️ Fix carousel scrolling in AttachmentCarousel
kidroca 155bf0e
⚡️ chore(AttachmentCarousel): fix formatting issues
kidroca a02bb10
Remove unused AttachmentCarouselCellRenderer.tsx
kidroca 3a1e687
🔄 Adjust scroll position on window width change in AttachmentCarousel
kidroca eed77be
Merge branch 'refs/heads/main' into kidroca/fix/carousel-opens-on-wro…
kidroca 3089399
Disable eslint rule for exhaustive-deps in AttachmentCarousel
kidroca 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
29 changes: 0 additions & 29 deletions
29
src/components/Attachments/AttachmentCarousel/AttachmentCarouselCellRenderer.tsx
This file was deleted.
Oops, something went wrong.
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,21 +1,23 @@ | ||||||
import isEqual from 'lodash/isEqual'; | ||||||
import React, {useCallback, useEffect, useRef, useState} from 'react'; | ||||||
import React, {useCallback, useEffect, useMemo, useState} from 'react'; | ||||||
import type {ListRenderItemInfo} from 'react-native'; | ||||||
import {FlatList, Keyboard, PixelRatio, View} from 'react-native'; | ||||||
import {Keyboard, PixelRatio, View} from 'react-native'; | ||||||
import {Gesture, GestureDetector} from 'react-native-gesture-handler'; | ||||||
import {withOnyx} from 'react-native-onyx'; | ||||||
import Animated, {scrollTo, useAnimatedRef} from 'react-native-reanimated'; | ||||||
import type {Attachment, AttachmentSource} from '@components/Attachments/types'; | ||||||
import BlockingView from '@components/BlockingViews/BlockingView'; | ||||||
import * as Illustrations from '@components/Icon/Illustrations'; | ||||||
import {useFullScreenContext} from '@components/VideoPlayerContexts/FullScreenContext'; | ||||||
import useLocalize from '@hooks/useLocalize'; | ||||||
import useTheme from '@hooks/useTheme'; | ||||||
import useThemeStyles from '@hooks/useThemeStyles'; | ||||||
import useWindowDimensions from '@hooks/useWindowDimensions'; | ||||||
import * as DeviceCapabilities from '@libs/DeviceCapabilities'; | ||||||
import Navigation from '@libs/Navigation/Navigation'; | ||||||
import variables from '@styles/variables'; | ||||||
import CONST from '@src/CONST'; | ||||||
import ONYXKEYS from '@src/ONYXKEYS'; | ||||||
import AttachmentCarouselCellRenderer from './AttachmentCarouselCellRenderer'; | ||||||
import CarouselActions from './CarouselActions'; | ||||||
import CarouselButtons from './CarouselButtons'; | ||||||
import CarouselItem from './CarouselItem'; | ||||||
|
@@ -29,16 +31,23 @@ const viewabilityConfig = { | |||||
itemVisiblePercentThreshold: 95, | ||||||
}; | ||||||
|
||||||
const MIN_FLING_VELOCITY = 500; | ||||||
|
||||||
function AttachmentCarousel({report, reportActions, parentReportActions, source, onNavigate, setDownloadButtonVisibility}: AttachmentCarouselProps) { | ||||||
const theme = useTheme(); | ||||||
const {translate} = useLocalize(); | ||||||
const {isSmallScreenWidth, windowWidth} = useWindowDimensions(); | ||||||
const styles = useThemeStyles(); | ||||||
const {isFullScreenRef} = useFullScreenContext(); | ||||||
const scrollRef = useRef<FlatList>(null); | ||||||
const scrollRef = useAnimatedRef<Animated.FlatList<ListRenderItemInfo<Attachment>>>(); | ||||||
|
||||||
const canUseTouchScreen = DeviceCapabilities.canUseTouchScreen(); | ||||||
|
||||||
const [containerWidth, setContainerWidth] = useState(0); | ||||||
const modalStyles = styles.centeredModalStyles(isSmallScreenWidth, true); | ||||||
const cellWidth = useMemo( | ||||||
() => PixelRatio.roundToNearestPixel(windowWidth - (modalStyles.marginHorizontal + modalStyles.borderWidth) * 2), | ||||||
[modalStyles.borderWidth, modalStyles.marginHorizontal, windowWidth], | ||||||
); | ||||||
const [page, setPage] = useState(0); | ||||||
const [attachments, setAttachments] = useState<Attachment[]>([]); | ||||||
const [activeSource, setActiveSource] = useState<AttachmentSource | null>(source); | ||||||
|
@@ -75,6 +84,17 @@ function AttachmentCarousel({report, reportActions, parentReportActions, source, | |||||
} | ||||||
}, [reportActions, parentReportActions, compareImage, report.parentReportActionID, attachments, setDownloadButtonVisibility, onNavigate]); | ||||||
|
||||||
// Scroll position is affected when window width is resized, so we readjust it on width changes | ||||||
useEffect(() => { | ||||||
if (attachments.length === 0 || scrollRef.current == null) { | ||||||
return; | ||||||
} | ||||||
|
||||||
scrollRef.current.scrollToIndex({index: page, animated: false}); | ||||||
// The hook is not supposed to run on page change, so we keep the page out of the dependencies | ||||||
// eslint-disable-next-line react-hooks/exhaustive-deps | ||||||
}, [cellWidth]); | ||||||
|
||||||
/** Updates the page state when the user navigates between attachments */ | ||||||
const updatePage = useCallback( | ||||||
({viewableItems}: UpdatePageProps) => { | ||||||
|
@@ -120,7 +140,7 @@ function AttachmentCarousel({report, reportActions, parentReportActions, source, | |||||
|
||||||
scrollRef.current.scrollToIndex({index: nextIndex, animated: canUseTouchScreen}); | ||||||
}, | ||||||
[attachments, canUseTouchScreen, isFullScreenRef, page], | ||||||
[attachments, canUseTouchScreen, isFullScreenRef, page, scrollRef], | ||||||
); | ||||||
|
||||||
const extractItemKey = useCallback( | ||||||
|
@@ -132,35 +152,55 @@ function AttachmentCarousel({report, reportActions, parentReportActions, source, | |||||
/** Calculate items layout information to optimize scrolling performance */ | ||||||
const getItemLayout = useCallback( | ||||||
(data: ArrayLike<Attachment> | null | undefined, index: number) => ({ | ||||||
length: containerWidth, | ||||||
offset: containerWidth * index, | ||||||
length: cellWidth, | ||||||
offset: cellWidth * index, | ||||||
index, | ||||||
}), | ||||||
[containerWidth], | ||||||
[cellWidth], | ||||||
); | ||||||
|
||||||
/** Defines how a single attachment should be rendered */ | ||||||
const renderItem = useCallback( | ||||||
({item}: ListRenderItemInfo<Attachment>) => ( | ||||||
<CarouselItem | ||||||
item={item} | ||||||
isFocused={activeSource === item.source} | ||||||
onPress={canUseTouchScreen ? () => setShouldShowArrows((oldState: boolean) => !oldState) : undefined} | ||||||
isModalHovered={shouldShowArrows} | ||||||
/> | ||||||
<View style={[styles.h100, {width: cellWidth}]}> | ||||||
<CarouselItem | ||||||
item={item} | ||||||
isFocused={activeSource === item.source} | ||||||
onPress={canUseTouchScreen ? () => setShouldShowArrows((oldState) => !oldState) : undefined} | ||||||
isModalHovered={shouldShowArrows} | ||||||
/> | ||||||
</View> | ||||||
), | ||||||
[activeSource, canUseTouchScreen, setShouldShowArrows, shouldShowArrows], | ||||||
[activeSource, canUseTouchScreen, cellWidth, setShouldShowArrows, shouldShowArrows, styles.h100], | ||||||
); | ||||||
/** Pan gesture handing swiping through attachments on touch screen devices */ | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. NAB
Suggested change
|
||||||
const pan = useMemo( | ||||||
() => | ||||||
Gesture.Pan() | ||||||
.enabled(canUseTouchScreen) | ||||||
.onUpdate(({translationX}) => scrollTo(scrollRef, page * cellWidth - translationX, 0, false)) | ||||||
.onEnd(({translationX, velocityX}) => { | ||||||
let newIndex; | ||||||
if (velocityX > MIN_FLING_VELOCITY) { | ||||||
// User flung to the right | ||||||
newIndex = Math.max(0, page - 1); | ||||||
} else if (velocityX < -MIN_FLING_VELOCITY) { | ||||||
// User flung to the left | ||||||
newIndex = Math.min(attachments.length - 1, page + 1); | ||||||
} else { | ||||||
// snap scroll position to the nearest cell (making sure it's within the bounds of the list) | ||||||
const delta = Math.round(-translationX / cellWidth); | ||||||
newIndex = Math.min(attachments.length - 1, Math.max(0, page + delta)); | ||||||
} | ||||||
|
||||||
scrollTo(scrollRef, newIndex * cellWidth, 0, true); | ||||||
}), | ||||||
[attachments.length, canUseTouchScreen, cellWidth, page, scrollRef], | ||||||
); | ||||||
|
||||||
return ( | ||||||
<View | ||||||
style={[styles.flex1, styles.attachmentCarouselContainer]} | ||||||
onLayout={({nativeEvent}) => { | ||||||
if (isFullScreenRef.current) { | ||||||
return; | ||||||
} | ||||||
setContainerWidth(PixelRatio.roundToNearestPixel(nativeEvent.layout.width)); | ||||||
}} | ||||||
onMouseEnter={() => !canUseTouchScreen && setShouldShowArrows(true)} | ||||||
onMouseLeave={() => !canUseTouchScreen && setShouldShowArrows(false)} | ||||||
> | ||||||
|
@@ -184,36 +224,26 @@ function AttachmentCarousel({report, reportActions, parentReportActions, source, | |||||
cancelAutoHideArrow={cancelAutoHideArrows} | ||||||
/> | ||||||
|
||||||
{containerWidth > 0 && ( | ||||||
<FlatList | ||||||
<GestureDetector gesture={pan}> | ||||||
<Animated.FlatList | ||||||
keyboardShouldPersistTaps="handled" | ||||||
horizontal | ||||||
decelerationRate="fast" | ||||||
showsHorizontalScrollIndicator={false} | ||||||
bounces={false} | ||||||
// Scroll only one image at a time no matter how fast the user swipes | ||||||
disableIntervalMomentum | ||||||
pagingEnabled | ||||||
snapToAlignment="start" | ||||||
snapToInterval={containerWidth} | ||||||
// Enable scrolling by swiping on mobile (touch) devices only | ||||||
// disable scroll for desktop/browsers because they add their scrollbars | ||||||
// Enable scrolling FlatList only when PDF is not in a zoomed state | ||||||
scrollEnabled={canUseTouchScreen} | ||||||
// scrolling is controlled by the pan gesture | ||||||
scrollEnabled={false} | ||||||
ref={scrollRef} | ||||||
initialScrollIndex={page} | ||||||
initialNumToRender={3} | ||||||
windowSize={5} | ||||||
maxToRenderPerBatch={CONST.MAX_TO_RENDER_PER_BATCH.CAROUSEL} | ||||||
data={attachments} | ||||||
CellRendererComponent={AttachmentCarouselCellRenderer} | ||||||
renderItem={renderItem} | ||||||
getItemLayout={getItemLayout} | ||||||
keyExtractor={extractItemKey} | ||||||
viewabilityConfig={viewabilityConfig} | ||||||
onViewableItemsChanged={updatePage} | ||||||
/> | ||||||
)} | ||||||
</GestureDetector> | ||||||
|
||||||
<CarouselActions onCycleThroughAttachments={cycleThroughAttachments} /> | ||||||
</> | ||||||
|
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.
Is this value custom or picked from FlatList?
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 custom value is used to gauge the speed of a user's swipe:
I determined this value through testing typical swiping motions intended to fling to the next image. However, I am open to suggestions for an optimized value. Given that Software Mansion is the creator of the gesture handler used here and is involved in this project, maybe we can give them a shout?