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

Jump to bottom

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 2 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
170 changes: 95 additions & 75 deletions src/components/AttachmentCarousel/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ import withLocalize, {withLocalizePropTypes} from '../withLocalize';
import compose from '../../libs/compose';
import withWindowDimensions from '../withWindowDimensions';
import reportPropTypes from '../../pages/reportPropTypes';
import BlockingView from '../BlockingViews/BlockingView';
import * as Illustrations from '../Icon/Illustrations';
import variables from '../../styles/variables';

const propTypes = {
/** source is used to determine the starting index in the array of attachments */
Expand All @@ -29,6 +32,9 @@ const propTypes = {
/** Callback to update the parent modal's state with a source and name from the attachments array */
onNavigate: PropTypes.func,

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

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

Expand All @@ -42,6 +48,7 @@ const defaultProps = {
source: '',
reportActions: {},
onNavigate: () => {},
toggleDownloadButtonVisibility: () => {},
};

class AttachmentCarousel extends React.Component {
Expand Down Expand Up @@ -190,12 +197,14 @@ class AttachmentCarousel extends React.Component {
}

const page = _.findIndex(attachments, (a) => a.source === this.props.source);
if (page === -1) {
throw new Error('Attachment not found');

if (page !== -1) {
// Update the parent modal's state with the source and name from the mapped attachments
this.props.onNavigate(attachments[page]);
}

// Update the parent modal's state with the source and name from the mapped attachments
this.props.onNavigate(attachments[page]);
// Update the download button visibility in the parent modal
this.props.toggleDownloadButtonVisibility(page !== -1);

return {
page,
Expand Down Expand Up @@ -302,82 +311,93 @@ class AttachmentCarousel extends React.Component {
onMouseEnter={() => !this.canUseTouchScreen && this.toggleArrowsVisibility(true)}
onMouseLeave={() => !this.canUseTouchScreen && this.toggleArrowsVisibility(false)}
>
{this.state.shouldShowArrow && (
{this.state.page === -1 ? (
<BlockingView
icon={Illustrations.ToddBehindCloud}
iconWidth={variables.modalTopIconWidth}
iconHeight={variables.modalTopIconHeight}
title={this.props.translate('notFound.notHere')}
/>
) : (
<>
{!isBackDisabled && (
<Tooltip text={this.props.translate('common.previous')}>
<View style={[styles.attachmentArrow, this.props.isSmallScreenWidth ? styles.l2 : styles.l8]}>
<Button
small
innerStyles={[styles.arrowIcon]}
icon={Expensicons.BackArrow}
iconFill={themeColors.text}
iconStyles={[styles.mr0]}
onPress={() => {
this.cycleThroughAttachments(-1);
this.autoHideArrow();
}}
onPressIn={this.cancelAutoHideArrow}
onPressOut={this.autoHideArrow}
/>
</View>
</Tooltip>
{this.state.shouldShowArrow && (
<>
{!isBackDisabled && (
<Tooltip text={this.props.translate('common.previous')}>
<View style={[styles.attachmentArrow, this.props.isSmallScreenWidth ? styles.l2 : styles.l8]}>
<Button
small
innerStyles={[styles.arrowIcon]}
icon={Expensicons.BackArrow}
iconFill={themeColors.text}
iconStyles={[styles.mr0]}
onPress={() => {
this.cycleThroughAttachments(-1);
this.autoHideArrow();
}}
onPressIn={this.cancelAutoHideArrow}
onPressOut={this.autoHideArrow}
/>
</View>
</Tooltip>
)}
{!isForwardDisabled && (
<Tooltip text={this.props.translate('common.next')}>
<View style={[styles.attachmentArrow, this.props.isSmallScreenWidth ? styles.r2 : styles.r8]}>
<Button
small
innerStyles={[styles.arrowIcon]}
icon={Expensicons.ArrowRight}
iconFill={themeColors.text}
iconStyles={[styles.mr0]}
onPress={() => {
this.cycleThroughAttachments(1);
this.autoHideArrow();
}}
onPressIn={this.cancelAutoHideArrow}
onPressOut={this.autoHideArrow}
/>
</View>
</Tooltip>
)}
</>
)}
{!isForwardDisabled && (
<Tooltip text={this.props.translate('common.next')}>
<View style={[styles.attachmentArrow, this.props.isSmallScreenWidth ? styles.r2 : styles.r8]}>
<Button
small
innerStyles={[styles.arrowIcon]}
icon={Expensicons.ArrowRight}
iconFill={themeColors.text}
iconStyles={[styles.mr0]}
onPress={() => {
this.cycleThroughAttachments(1);
this.autoHideArrow();
}}
onPressIn={this.cancelAutoHideArrow}
onPressOut={this.autoHideArrow}
/>
</View>
</Tooltip>

{this.state.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={this.state.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={this.canUseTouchScreen && !this.state.isZoomed}
ref={this.scrollRef}
initialScrollIndex={this.state.page}
initialNumToRender={3}
windowSize={5}
maxToRenderPerBatch={3}
data={this.state.attachments}
CellRendererComponent={this.renderCell}
renderItem={this.renderItem}
getItemLayout={this.getItemLayout}
keyExtractor={(item) => item.source}
viewabilityConfig={this.viewabilityConfig}
onViewableItemsChanged={this.updatePage}
/>
)}
</>
)}

{this.state.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={this.state.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={this.canUseTouchScreen && !this.state.isZoomed}
ref={this.scrollRef}
initialScrollIndex={this.state.page}
initialNumToRender={3}
windowSize={5}
maxToRenderPerBatch={3}
data={this.state.attachments}
CellRendererComponent={this.renderCell}
renderItem={this.renderItem}
getItemLayout={this.getItemLayout}
keyExtractor={(item) => item.source}
viewabilityConfig={this.viewabilityConfig}
onViewableItemsChanged={this.updatePage}
/>
<CarouselActions onCycleThroughAttachments={this.cycleThroughAttachments} />
</>
)}

<CarouselActions onCycleThroughAttachments={this.cycleThroughAttachments} />
</View>
);
}
Expand Down
18 changes: 17 additions & 1 deletion src/components/AttachmentModal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,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 @@ -136,6 +137,20 @@ function AttachmentModal(props) {
// eslint-disable-next-line react-hooks/exhaustive-deps
[props.translate],
);

/**
Copy link
Collaborator

@mananjadhav mananjadhav Jul 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

nab, I don't think we need this comment.

* Toggles the visibility of the download button
* @param {Boolean} shouldShowButton
*/
const toggleDownloadButtonVisibility = useCallback(
ahmedGaber93 marked this conversation as resolved.
Show resolved Hide resolved
(shouldShowButton) => {
if (shouldShowDownloadButton === shouldShowButton) {
return;
}
setShouldShowDownloadButton(shouldShowButton);
},
[shouldShowDownloadButton],
);
/**
* Download the currently viewed attachment.
*/
Expand Down Expand Up @@ -310,7 +325,7 @@ function AttachmentModal(props) {
<HeaderWithBackButton
title={props.headerTitle || props.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 @@ -324,6 +339,7 @@ function AttachmentModal(props) {
onNavigate={onNavigate}
source={props.source}
onToggleKeyboard={updateConfirmButtonVisibility}
toggleDownloadButtonVisibility={toggleDownloadButtonVisibility}
/>
) : (
Boolean(sourceForAttachmentView) &&
Expand Down
Loading