-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Fix Opened offline attachment directed to conversation page on online #49832
Changes from 14 commits
48af2f2
6ece814
19db64e
34ba2c7
2f21ca5
67c1f2b
f95d459
de935c4
ce69418
613d8f3
f458e85
15ac1ed
b42d3cd
8ef2085
e30a878
ed960b1
ca1e6d5
4167773
6741ab6
3e2e8d4
fbd028f
9a844fa
3c10da9
aaea3ba
04d8e9f
78a920e
d76d54d
aebb27f
8e34c56
7bb7bb2
eb33707
2f7068a
f104033
33cd212
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,7 +5,7 @@ import type {ListRenderItemInfo} from 'react-native'; | |
import {Keyboard, PixelRatio, View} from 'react-native'; | ||
import type {GestureType} from 'react-native-gesture-handler'; | ||
import {Gesture, GestureDetector} from 'react-native-gesture-handler'; | ||
import {withOnyx} from 'react-native-onyx'; | ||
import {useOnyx} from 'react-native-onyx'; | ||
import Animated, {scrollTo, useAnimatedRef, useSharedValue} from 'react-native-reanimated'; | ||
import type {Attachment, AttachmentSource} from '@components/Attachments/types'; | ||
import BlockingView from '@components/BlockingViews/BlockingView'; | ||
|
@@ -26,7 +26,7 @@ import CarouselButtons from './CarouselButtons'; | |
import CarouselItem from './CarouselItem'; | ||
import extractAttachments from './extractAttachments'; | ||
import AttachmentCarouselPagerContext from './Pager/AttachmentCarouselPagerContext'; | ||
import type {AttachmentCaraouselOnyxProps, AttachmentCarouselProps, UpdatePageProps} from './types'; | ||
import type {AttachmentCarouselProps, UpdatePageProps} from './types'; | ||
import useCarouselArrows from './useCarouselArrows'; | ||
import useCarouselContextEvents from './useCarouselContextEvents'; | ||
|
||
|
@@ -38,7 +38,7 @@ const viewabilityConfig = { | |
|
||
const MIN_FLING_VELOCITY = 500; | ||
|
||
function AttachmentCarousel({report, reportActions, parentReportActions, source, onNavigate, setDownloadButtonVisibility, type, accountID, onClose}: AttachmentCarouselProps) { | ||
function AttachmentCarousel({report, source, onNavigate, setDownloadButtonVisibility, type, accountID, onClose}: AttachmentCarouselProps) { | ||
const theme = useTheme(); | ||
const {translate} = useLocalize(); | ||
const {windowWidth} = useWindowDimensions(); | ||
|
@@ -48,7 +48,8 @@ function AttachmentCarousel({report, reportActions, parentReportActions, source, | |
const scrollRef = useAnimatedRef<Animated.FlatList<ListRenderItemInfo<Attachment>>>(); | ||
const nope = useSharedValue(false); | ||
const pagerRef = useRef<GestureType>(null); | ||
|
||
const [parentReportActions] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report.parentReportID}`, {canEvict: false}); | ||
const [reportActions] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report.reportID}`, {canEvict: false}); | ||
const canUseTouchScreen = DeviceCapabilities.canUseTouchScreen(); | ||
|
||
const modalStyles = styles.centeredModalStyles(shouldUseNarrowLayout, true); | ||
|
@@ -88,10 +89,16 @@ function AttachmentCarousel({report, reportActions, parentReportActions, source, | |
return; | ||
} | ||
|
||
const initialPage = targetAttachments.findIndex(compareImage); | ||
let initialPage = targetAttachments.findIndex(compareImage); | ||
const prevInitialPage = attachments.findIndex(compareImage); | ||
|
||
// If no matching attachment is found in targetAttachments but found in attachments, update initialPage | ||
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. This comment is explaining the what not the why. Let's remove it 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. done |
||
if (initialPage === -1 && prevInitialPage !== -1 && targetAttachments[prevInitialPage]) { | ||
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. Can we avoid comparing with -1 and instead just access the attachment array directly if (!targetAttachments[attachmentIndex] && targetAttachments[prevAttachmentIndex]) 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. done |
||
initialPage = prevInitialPage; | ||
} | ||
|
||
// Dismiss the modal when deleting an attachment during its display in preview. | ||
if (initialPage === -1 && attachments.find(compareImage)) { | ||
// If no matching attachment with the same index, dismiss the modal | ||
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. Same ^ 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. done |
||
if (initialPage === -1 && prevInitialPage !== -1) { | ||
Navigation.dismissModal(); | ||
} else { | ||
setPage(initialPage); | ||
|
@@ -306,13 +313,4 @@ function AttachmentCarousel({report, reportActions, parentReportActions, source, | |
|
||
AttachmentCarousel.displayName = 'AttachmentCarousel'; | ||
|
||
export default withOnyx<AttachmentCarouselProps, AttachmentCaraouselOnyxProps>({ | ||
parentReportActions: { | ||
key: ({report}) => `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report.parentReportID}`, | ||
canEvict: false, | ||
}, | ||
reportActions: { | ||
key: ({report}) => `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report.reportID}`, | ||
canEvict: false, | ||
}, | ||
})(AttachmentCarousel); | ||
export default AttachmentCarousel; |
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.
I feel that this naming is more correct.
findIndex
returns an index and not a page. The index is used as the page id.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.
done