Skip to content
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

Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
48af2f2
change attachment target to prevInitialPageRef when current viewed at…
wildan-m Sep 23, 2024
6ece814
don't dismiss only when the prev attachment with the same initialPage…
wildan-m Sep 23, 2024
19db64e
Merge branch 'main' of https://github.com/wildan-m/App into wildan/fi…
wildan-m Sep 26, 2024
34ba2c7
Merge branch 'main' of https://github.com/wildan-m/App into wildan/fi…
wildan-m Sep 26, 2024
2f21ca5
remove unnecessary code
wildan-m Sep 27, 2024
67c1f2b
adjust comment
wildan-m Sep 27, 2024
f95d459
fix lint, replace deprecated withOnyx
wildan-m Sep 27, 2024
de935c4
implement for native
wildan-m Sep 27, 2024
ce69418
lint fix, replace deprecate withOnyx
wildan-m Sep 27, 2024
613d8f3
typecheck fix
wildan-m Sep 27, 2024
f458e85
remove unused code
wildan-m Sep 27, 2024
15ac1ed
Update src/components/Attachments/AttachmentCarousel/index.tsx
wildan-m Sep 28, 2024
b42d3cd
refactor
wildan-m Sep 28, 2024
8ef2085
refactor for native
wildan-m Sep 28, 2024
e30a878
Merge branch 'main' of https://github.com/wildan-m/App into wildan/fi…
wildan-m Sep 29, 2024
ed960b1
remove unnecessary comment
wildan-m Sep 29, 2024
ca1e6d5
access the array directly instead of checking -1 in index
wildan-m Sep 29, 2024
4167773
adjust comment position
wildan-m Sep 29, 2024
6741ab6
Merge branch 'main' of https://github.com/wildan-m/App into wildan/fi…
wildan-m Sep 30, 2024
3e2e8d4
resolve issue incorrect newIndex on onPageSelected event
wildan-m Sep 30, 2024
fbd028f
refine comment
wildan-m Sep 30, 2024
9a844fa
refactor
wildan-m Sep 30, 2024
3c10da9
fix issue attachment briefly changed before dismiss
wildan-m Sep 30, 2024
aaea3ba
Merge branch 'main' of https://github.com/wildan-m/App into wildan/fi…
wildan-m Oct 1, 2024
04d8e9f
Refactor
wildan-m Oct 1, 2024
78a920e
Merge branch 'main' of https://github.com/wildan-m/App into wildan/fi…
wildan-m Oct 2, 2024
d76d54d
revert pager key solution
wildan-m Oct 2, 2024
aebb27f
fix lint
wildan-m Oct 2, 2024
8e34c56
fix lint
wildan-m Oct 2, 2024
7bb7bb2
revert compare with -1 after new eslint rule
wildan-m Oct 2, 2024
eb33707
sync web code with native
wildan-m Oct 2, 2024
2f7068a
-1 check for array access using .at
wildan-m Oct 2, 2024
f104033
Merge branch 'main' of https://github.com/wildan-m/App into wildan/fi…
wildan-m Oct 2, 2024
33cd212
Refine comment, add why point
wildan-m Oct 3, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 15 additions & 16 deletions src/components/Attachments/AttachmentCarousel/index.native.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import React, {useCallback, useEffect, useRef, useState} from 'react';
import {Keyboard, View} from 'react-native';
import {withOnyx} from 'react-native-onyx';
import {useOnyx} from 'react-native-onyx';
import type {Attachment, AttachmentSource} from '@components/Attachments/types';
import BlockingView from '@components/BlockingViews/BlockingView';
import FullScreenLoadingIndicator from '@components/FullscreenLoadingIndicator';
Expand All @@ -15,13 +15,15 @@ import CarouselButtons from './CarouselButtons';
import extractAttachments from './extractAttachments';
import type {AttachmentCarouselPagerHandle} from './Pager';
import AttachmentCarouselPager from './Pager';
import type {AttachmentCaraouselOnyxProps, AttachmentCarouselProps} from './types';
import type {AttachmentCarouselProps} from './types';
import useCarouselArrows from './useCarouselArrows';

function AttachmentCarousel({report, reportActions, parentReportActions, source, onNavigate, setDownloadButtonVisibility, onClose, type, accountID}: AttachmentCarouselProps) {
function AttachmentCarousel({report, source, onNavigate, setDownloadButtonVisibility, onClose, type, accountID}: AttachmentCarouselProps) {
const styles = useThemeStyles();
const {translate} = useLocalize();
const pagerRef = useRef<AttachmentCarouselPagerHandle>(null);
const [parentReportActions] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report.parentReportID}`, {canEvict: false});
const [reportActions] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report.reportID}`, {canEvict: false});
const [page, setPage] = useState<number>();
const [attachments, setAttachments] = useState<Attachment[]>([]);
const {shouldShowArrows, setShouldShowArrows, autoHideArrows, cancelAutoHideArrows} = useCarouselArrows();
Expand All @@ -38,10 +40,16 @@ function AttachmentCarousel({report, reportActions, parentReportActions, source,
targetAttachments = extractAttachments(CONST.ATTACHMENT_TYPE.REPORT, {parentReportAction, reportActions});
}

const initialPage = targetAttachments.findIndex(compareImage);
let initialPage = targetAttachments.findIndex(compareImage);
const prevInitialPage = attachments.findIndex(compareImage);

// Dismiss the modal when deleting an attachment during its display in preview.
if (initialPage === -1 && attachments.find(compareImage)) {
// If no matching attachment is found in targetAttachments but found in attachments, update initialPage
if (initialPage === -1 && prevInitialPage !== -1 && targetAttachments[prevInitialPage]) {
initialPage = prevInitialPage;
}

// If no matching attachment with the same index, dismiss the modal
if (initialPage === -1 && prevInitialPage !== -1) {
Navigation.dismissModal();
} else {
setPage(initialPage);
Expand Down Expand Up @@ -144,13 +152,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;
32 changes: 15 additions & 17 deletions src/components/Attachments/AttachmentCarousel/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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';

Expand All @@ -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();
Expand All @@ -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);
Expand Down Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let initialPage = targetAttachments.findIndex(compareImage);
const prevInitialPage = attachments.findIndex(compareImage);
let attachmentIndex = targetAttachments.findIndex(compareImage);
const prevAttachmentIndex = attachments.findIndex(compareImage);

I feel that this naming is more correct. findIndex returns an index and not a page. The index is used as the page id.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


// If no matching attachment is found in targetAttachments but found in attachments, update initialPage
Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

if (initialPage === -1 && prevInitialPage !== -1 && targetAttachments[prevInitialPage]) {
Copy link
Contributor

Choose a reason for hiding this comment

The 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])

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same ^

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

if (initialPage === -1 && prevInitialPage !== -1) {
Navigation.dismissModal();
} else {
setPage(initialPage);
Expand Down Expand Up @@ -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;
15 changes: 3 additions & 12 deletions src/components/Attachments/AttachmentCarousel/types.ts
Original file line number Diff line number Diff line change
@@ -1,23 +1,14 @@
import type {ViewToken} from 'react-native';
import type {OnyxEntry} from 'react-native-onyx';
import type {ValueOf} from 'type-fest';
import type {Attachment, AttachmentSource} from '@components/Attachments/types';
import type CONST from '@src/CONST';
import type {Report, ReportActions} from '@src/types/onyx';
import type {Report} from '@src/types/onyx';

type UpdatePageProps = {
viewableItems: ViewToken[];
};

type AttachmentCaraouselOnyxProps = {
/** Object of report actions for this report */
reportActions: OnyxEntry<ReportActions>;

/** The report actions of the parent report */
parentReportActions: OnyxEntry<ReportActions>;
};

type AttachmentCarouselProps = AttachmentCaraouselOnyxProps & {
type AttachmentCarouselProps = {
/** Source is used to determine the starting index in the array of attachments */
source: AttachmentSource;

Expand All @@ -40,4 +31,4 @@ type AttachmentCarouselProps = AttachmentCaraouselOnyxProps & {
onClose: () => void;
};

export type {AttachmentCarouselProps, UpdatePageProps, AttachmentCaraouselOnyxProps};
export type {AttachmentCarouselProps, UpdatePageProps};
Loading