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.

Sign up for GitHub

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

display the not found page when attachment is deleted. #23143

Merged
merged 15 commits into from
Aug 17, 2023
Merged

display the not found page when attachment is deleted. #23143

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
1feeb80
display not found page when attachment is deleted.
ahmedGaber93 Jul 18, 2023
c9e86db
prettier
ahmedGaber93 Jul 18, 2023
1974052
change toggleDownloadButtonVisibility method name to setDownloadButto…
ahmedGaber93 Jul 19, 2023
8cf2cdb
fix attachment deep link not work in native ios and android
ahmedGaber93 Jul 25, 2023
9419385
Merge branch 'main' into issue-22261
ahmedGaber93 Jul 27, 2023
0d20879
fix merge conflict
ahmedGaber93 Jul 27, 2023
2a7b2e5
Merge branch 'main' into issue-22261
ahmedGaber93 Aug 4, 2023
533a553
fix dismiss modal when deleting attachment
ahmedGaber93 Aug 4, 2023
4aff8e2
fix dismiss modal when deleting attachment
ahmedGaber93 Aug 4, 2023
2fa9a37
prettier
ahmedGaber93 Aug 4, 2023
b4c04b9
remove uplicated tryResolveUrlFromApiRoot
ahmedGaber93 Aug 7, 2023
0dad55b
Merge branch 'main' into issue-22261
ahmedGaber93 Aug 7, 2023
5f2b425
fix conflict
ahmedGaber93 Aug 8, 2023
b9af016
improve comments
ahmedGaber93 Aug 9, 2023
a0962d1
improve comments
ahmedGaber93 Aug 11, 2023
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
17 changes: 15 additions & 2 deletions src/components/AttachmentModal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import HeaderGap from './HeaderGap';
import SafeAreaConsumer from './SafeAreaConsumer';
import addEncryptedAuthTokenToURL from '../libs/addEncryptedAuthTokenToURL';
import reportPropTypes from '../pages/reportPropTypes';
import tryResolveUrlFromApiRoot from '../libs/tryResolveUrlFromApiRoot';

/**
* Modal render prop component that exposes modal launching triggers that can be used
Expand Down Expand Up @@ -99,6 +100,7 @@ function AttachmentModal(props) {
const [modalType, setModalType] = useState(CONST.MODAL.MODAL_TYPE.CENTERED_UNSWIPEABLE);
const [isConfirmButtonDisabled, setIsConfirmButtonDisabled] = useState(false);
const [confirmButtonFadeAnimation] = useState(new Animated.Value(1));
const [shouldShowDownloadButton, setShouldShowDownloadButton] = React.useState(true);
const [file, setFile] = useState(
props.originalFileName
? {
Expand Down Expand Up @@ -138,6 +140,16 @@ function AttachmentModal(props) {
[translate],
);

const setDownloadButtonVisibility = useCallback(
(shouldShowButton) => {
if (shouldShowDownloadButton === shouldShowButton) {
return;
}
setShouldShowDownloadButton(shouldShowButton);
},
[shouldShowDownloadButton],
);

/**
* Download the currently viewed attachment.
*/
Expand Down Expand Up @@ -330,7 +342,7 @@ function AttachmentModal(props) {
<HeaderWithBackButton
title={props.headerTitle || translate('common.attachment')}
shouldShowBorderBottom
shouldShowDownloadButton={props.allowDownload}
shouldShowDownloadButton={props.allowDownload && shouldShowDownloadButton}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am wondering if this can be just replaced with state, and props.allowDownload can be used in the useCallback?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think using props.allowDownload in useCallback dependencies will trigger change definition for method setDownloadButtonVisibility not shouldShowDownloadButton value.

but we can use it in useEffect.

const [shouldShowDownloadButton, setShouldShowDownloadButton] = React.useState(props.allowDownload);

useEffect(() => {
  if (shouldShowDownloadButton === props.allowDownload) {
      return;
  }
  setShouldShowDownloadButton(props.allowDownload);
}, [props.allowDownload]);

what do you think? @mananjadhav

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can ignore this change.

onDownloadButtonPress={() => downloadAttachment(source)}
shouldShowCloseButton={!props.isSmallScreenWidth}
shouldShowBackButton={props.isSmallScreenWidth}
Expand All @@ -342,9 +354,10 @@ function AttachmentModal(props) {
<AttachmentCarousel
report={props.report}
onNavigate={onNavigate}
source={props.source}
source={tryResolveUrlFromApiRoot(props.source)}
Copy link
Contributor

Choose a reason for hiding this comment

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

@ahmedGaber93 Can you please help explain why do we need to use tryResolveUrlFromApiRoot here? Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tienifr
We decide here #23143 (comment) to used it, and here #23143 (comment) to move it in this place.

onClose={closeModal}
onToggleKeyboard={updateConfirmButtonVisibility}
setDownloadButtonVisibility={setDownloadButtonVisibility}
/>
) : (
Boolean(sourceForAttachmentView) &&
Expand Down
4 changes: 4 additions & 0 deletions src/components/Attachments/AttachmentCarousel/attachmentCarouselPropTypes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@ const propTypes = {
/** Callback to close carousel when user swipes down (on native) */
onClose: PropTypes.func,

/** Function to change the download button Visibility */
setDownloadButtonVisibility: PropTypes.func,
ahmedGaber93 marked this conversation as resolved.
Show resolved Hide resolved

/** Object of report actions for this report */
reportActions: PropTypes.shape(reportActionPropTypes),

Expand All @@ -24,6 +27,7 @@ const defaultProps = {
reportActions: {},
onNavigate: () => {},
onClose: () => {},
setDownloadButtonVisibility: () => {},
};

export {propTypes, defaultProps};
30 changes: 4 additions & 26 deletions src/components/Attachments/AttachmentCarousel/extractAttachmentsFromReport.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,16 @@ import _ from 'underscore';
import * as ReportActionsUtils from '../../../libs/ReportActionsUtils';
import CONST from '../../../CONST';
import tryResolveUrlFromApiRoot from '../../../libs/tryResolveUrlFromApiRoot';
import Navigation from '../../../libs/Navigation/Navigation';

/**
* Constructs the initial component state from report actions
* @param {Object} report
* @param {Array} reportActions
* @param {String} source
* @returns {{attachments: Array, initialPage: Number, initialItem: Object, initialActiveSource: String}}
* @returns {Array}
*/
function extractAttachmentsFromReport(report, reportActions, source) {
function extractAttachmentsFromReport(report, reportActions) {
const actions = [ReportActionsUtils.getParentReportAction(report), ...ReportActionsUtils.getSortedReportActions(_.values(reportActions))];
let attachments = [];
const attachments = [];

const htmlParser = new HtmlParser({
onopentag: (name, attribs) => {
Expand Down Expand Up @@ -42,27 +40,7 @@ function extractAttachmentsFromReport(report, reportActions, source) {
});
htmlParser.end();

attachments = attachments.reverse();

const initialPage = _.findIndex(attachments, (a) => a.source === source);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why couldn't we have this logic here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need only to move Navigation.dismissModal() out of this function, but other PRS renamed the file createInitialState to extractAttachmentsFromReport, so I think it will be more clean if this function return only attachments array to be suitable for its name, but we can keep them.

if (initialPage === -1) {
Navigation.dismissModal();
return {
attachments: [],
initialPage: 0,
initialItem: undefined,
initialActiveSource: null,
};
}

const initialItem = attachments[initialPage];

return {
attachments,
initialPage,
initialItem,
initialActiveSource: initialItem.source,
};
return attachments.reverse();
}

export default extractAttachmentsFromReport;
134 changes: 81 additions & 53 deletions src/components/Attachments/AttachmentCarousel/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, {useRef, useCallback, useState, useEffect, useMemo} from 'react';
import React, {useRef, useCallback, useState, useEffect} from 'react';
import {View, FlatList, PixelRatio, Keyboard} from 'react-native';
import {withOnyx} from 'react-native-onyx';
import _ from 'underscore';
Expand All @@ -15,6 +15,10 @@ import withLocalize from '../../withLocalize';
import compose from '../../../libs/compose';
import useCarouselArrows from './useCarouselArrows';
import useWindowDimensions from '../../../hooks/useWindowDimensions';
import Navigation from '../../../libs/Navigation/Navigation';
import BlockingView from '../../BlockingViews/BlockingView';
import * as Illustrations from '../../Icon/Illustrations';
import variables from '../../../styles/variables';

const canUseTouchScreen = DeviceCapabilities.canUseTouchScreen();
const viewabilityConfig = {
Expand All @@ -23,24 +27,37 @@ const viewabilityConfig = {
itemVisiblePercentThreshold: 95,
};

function AttachmentCarousel({report, reportActions, source, onNavigate}) {
function AttachmentCarousel({report, reportActions, source, onNavigate, setDownloadButtonVisibility, translate}) {
const scrollRef = useRef(null);

const {windowWidth, isSmallScreenWidth} = useWindowDimensions();

const {attachments, initialPage, initialActiveSource, initialItem} = useMemo(() => extractAttachmentsFromReport(report, reportActions, source), [report, reportActions, source]);
const [containerWidth, setContainerWidth] = useState(0);
const [page, setPage] = useState(0);
const [attachments, setAttachments] = useState([]);
const [activeSource, setActiveSource] = useState(source);
const [shouldShowArrows, setShouldShowArrows, autoHideArrows, cancelAutoHideArrows] = useCarouselArrows();

useEffect(() => {
// Update the parent modal's state with the source and name from the mapped attachments
if (!initialItem) return;
onNavigate(initialItem);
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [initialItem]);
const attachmentsFromReport = extractAttachmentsFromReport(report, reportActions);

const [containerWidth, setContainerWidth] = useState(0);
const [page, setPage] = useState(initialPage);
const [activeSource, setActiveSource] = useState(initialActiveSource);
const [shouldShowArrows, setShouldShowArrows, autoHideArrows, cancelAutoHideArrows] = useCarouselArrows();
const initialPage = _.findIndex(attachmentsFromReport, (a) => a.source === source);

// Dismiss the modal when deleting an attachment during its display in preview.
if (initialPage === -1 && _.find(attachments, (a) => a.source === source)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just looking at the code I am not able to make out why did couldn't go in a common method? What's the difference between index.js and index.native.js, could merge this into a common method for both the platforms?

Copy link
Contributor Author

@ahmedGaber93 ahmedGaber93 Aug 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm… I thought about this, but I can't create a valuable function.
We need 2 variables from those two lines initialPage and isAttachmentDeletingInLivePreview.

// I think we need to use `_.find` twice to make the function readable, but as you comment before we are not sure if _.find for a lot of attachments could be expensive or not?
function isAttachmentDeletingInLivePreview(prevAttachments, nextAttachments, activeSource){
    return _.find(prevAttachments, (a) => a.source === activeSource) 
        && !_.find(nextAttachments, (a) => a.source === activeSource)
}

// i am not sure if this need to be a function.
function getInitialPage(attachments, source){
    return _.findIndex(attachments, (a) => a.source === source);
}

What do you think, or suggest about the desired function? @mananjadhav

Navigation.dismissModal();
} else {
setPage(initialPage);
setAttachments(attachmentsFromReport);

// Update the download button visibility in the parent modal
ahmedGaber93 marked this conversation as resolved.
Show resolved Hide resolved
setDownloadButtonVisibility(initialPage !== -1);

// Update the parent modal's state with the source and name from the mapped attachments
ahmedGaber93 marked this conversation as resolved.
Show resolved Hide resolved
if (!_.isUndefined(attachmentsFromReport[initialPage])) onNavigate(attachmentsFromReport[initialPage]);
}
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [report, reportActions, source]);

/**
* Updates the page state when the user navigates between attachments
Expand Down Expand Up @@ -153,49 +170,60 @@ function AttachmentCarousel({report, reportActions, source, onNavigate}) {
onMouseEnter={() => !canUseTouchScreen && setShouldShowArrows(true)}
onMouseLeave={() => !canUseTouchScreen && setShouldShowArrows(false)}
>
<CarouselButtons
shouldShowArrows={shouldShowArrows}
page={page}
attachments={attachments}
onBack={() => cycleThroughAttachments(-1)}
onForward={() => cycleThroughAttachments(1)}
autoHideArrow={autoHideArrows}
cancelAutoHideArrow={cancelAutoHideArrows}
/>

{containerWidth > 0 && (
<FlatList
keyboardShouldPersistTaps="handled"
listKey="AttachmentCarousel"
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}
ref={scrollRef}
initialScrollIndex={page}
initialNumToRender={3}
windowSize={5}
maxToRenderPerBatch={3}
data={attachments}
CellRendererComponent={renderCell}
renderItem={renderItem}
getItemLayout={getItemLayout}
keyExtractor={(item) => item.source}
viewabilityConfig={viewabilityConfig}
onViewableItemsChanged={updatePage.current}
{page === -1 ? (
<BlockingView
icon={Illustrations.ToddBehindCloud}
iconWidth={variables.modalTopIconWidth}
iconHeight={variables.modalTopIconHeight}
title={translate('notFound.notHere')}
/>
) : (
<>
<CarouselButtons
shouldShowArrows={shouldShowArrows}
page={page}
attachments={attachments}
onBack={() => cycleThroughAttachments(-1)}
onForward={() => cycleThroughAttachments(1)}
autoHideArrow={autoHideArrows}
cancelAutoHideArrow={cancelAutoHideArrows}
/>

{containerWidth > 0 && (
<FlatList
keyboardShouldPersistTaps="handled"
listKey="AttachmentCarousel"
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}
ref={scrollRef}
initialScrollIndex={page}
initialNumToRender={3}
windowSize={5}
maxToRenderPerBatch={3}
data={attachments}
CellRendererComponent={renderCell}
renderItem={renderItem}
getItemLayout={getItemLayout}
keyExtractor={(item) => item.source}
viewabilityConfig={viewabilityConfig}
onViewableItemsChanged={updatePage.current}
/>
)}

<CarouselActions onCycleThroughAttachments={cycleThroughAttachments} />
</>
)}

<CarouselActions onCycleThroughAttachments={cycleThroughAttachments} />
</View>
);
}
Expand Down
Loading