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: IOU Scan - In dark mode, the damaged PDF - file is barely visible. #40607

Merged
merged 19 commits into from
Jun 5, 2024
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
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
12 changes: 12 additions & 0 deletions assets/images/receipt-slash.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
2 changes: 1 addition & 1 deletion src/components/AttachmentPicker/index.native.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,7 @@ function AttachmentPicker({type = CONST.ATTACHMENT_PICKER_TYPE.FILE, children, s
* An attachment error dialog when user selected malformed images
*/
const showImageCorruptionAlert = useCallback(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

NAB: If we replace the wording from Image to Attachment for the translate key, should we change the function name to showAttachmentCorruptionAlert ?

Alert.alert(translate('attachmentPicker.attachmentError'), translate('attachmentPicker.errorWhileSelectingCorruptedImage'));
Alert.alert(translate('attachmentPicker.attachmentError'), translate('attachmentPicker.errorWhileSelectingCorruptedAttachment'));
}, [translate]);

/**
Expand Down
2 changes: 2 additions & 0 deletions src/components/Icon/Expensicons.ts
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,7 @@ import QrCode from '@assets/images/qrcode.svg';
import QuestionMark from '@assets/images/question-mark-circle.svg';
import ReceiptScan from '@assets/images/receipt-scan.svg';
import ReceiptSearch from '@assets/images/receipt-search.svg';
import ReceiptSlash from '@assets/images/receipt-slash.svg';
import Receipt from '@assets/images/receipt.svg';
import RemoveMembers from '@assets/images/remove-members.svg';
import Rotate from '@assets/images/rotate-image.svg';
Expand Down Expand Up @@ -298,6 +299,7 @@ export {
QuestionMark,
Receipt,
ReceiptScan,
ReceiptSlash,
RemoveMembers,
ReceiptSearch,
Rotate,
Expand Down
16 changes: 13 additions & 3 deletions src/components/MoneyRequestConfirmationList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -325,6 +325,7 @@ function MoneyRequestConfirmationList({
const [didConfirmSplit, setDidConfirmSplit] = useState(false);

const [isAttachmentInvalid, setIsAttachmentInvalid] = useState(false);
const [invalidAttachmentPromt, setInvalidAttachmentPromt] = useState(translate('attachmentPicker.protectedPDFNotSupported'));

const navigateBack = useCallback(
() => Navigation.goBack(ROUTES.MONEY_REQUEST_CREATE_TAB_SCAN.getRoute(CONST.IOU.ACTION.CREATE, iouType, transactionID, reportID)),
Expand Down Expand Up @@ -1096,7 +1097,14 @@ function MoneyRequestConfirmationList({
previewSourceURL={resolvedReceiptImage as string}
// We don't support scanning password protected PDF receipt
enabled={!isAttachmentInvalid}
onPassword={() => setIsAttachmentInvalid(true)}
onPassword={() => {
setIsAttachmentInvalid(true);
setInvalidAttachmentPromt(translate('attachmentPicker.protectedPDFNotSupported'));
}}
onLoadError={() => {
setInvalidAttachmentPromt(translate('attachmentPicker.errorWhileSelectingCorruptedAttachment'));
setIsAttachmentInvalid(true);
}}
/>
) : (
<ReceiptImage
Expand Down Expand Up @@ -1125,6 +1133,7 @@ function MoneyRequestConfirmationList({
receiptThumbnail,
fileExtension,
isDistanceRequest,
translate,
],
);

Expand Down Expand Up @@ -1181,11 +1190,11 @@ function MoneyRequestConfirmationList({
)}
<View style={[styles.mb5]}>{shouldShowAllFields && supplementaryFields}</View>
<ConfirmModal
title={translate('attachmentPicker.wrongFileType')}
title={translate('attachmentPicker.attachmentError')}
onConfirm={navigateBack}
onCancel={navigateBack}
isVisible={isAttachmentInvalid}
prompt={translate('attachmentPicker.protectedPDFNotSupported')}
prompt={invalidAttachmentPromt}
confirmText={translate('common.close')}
shouldShowCancelButton={false}
/>
Expand Down Expand Up @@ -1220,6 +1229,7 @@ function MoneyRequestConfirmationList({
transaction,
transactionID,
translate,
invalidAttachmentPromt,
],
);

Expand Down
33 changes: 25 additions & 8 deletions src/components/PDFThumbnail/index.native.tsx
Original file line number Diff line number Diff line change
@@ -1,19 +1,25 @@
import React from 'react';
import React, {useState} from 'react';
import {View} from 'react-native';
import Pdf from 'react-native-pdf';
import FullScreenLoadingIndicator from '@components/FullscreenLoadingIndicator';
import Icon from '@components/Icon';
import * as Expensicons from '@components/Icon/Expensicons';
import useTheme from '@hooks/useTheme';
import useThemeStyles from '@hooks/useThemeStyles';
import addEncryptedAuthTokenToURL from '@libs/addEncryptedAuthTokenToURL';
import variables from '@styles/variables';
import type PDFThumbnailProps from './types';

function PDFThumbnail({previewSourceURL, style, isAuthTokenRequired = false, enabled = true, onPassword}: PDFThumbnailProps) {
function PDFThumbnail({previewSourceURL, style, isAuthTokenRequired = false, enabled = true, onPassword, onLoadError}: PDFThumbnailProps) {
const styles = useThemeStyles();
const theme = useTheme();
const sizeStyles = [styles.w100, styles.h100];
const [hasError, setHasError] = useState(false);

Copy link
Contributor

Choose a reason for hiding this comment

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

hasError variable name doesn't look good here. Instead we can use something like failedToLoad @hayata-suenaga @Krishna2323

Copy link
Contributor

Choose a reason for hiding this comment

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

failedToLoad sounds good 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated.

return (
<View style={[style, styles.overflowHidden]}>
<View style={[sizeStyles, styles.alignItemsCenter, styles.justifyContentCenter]}>
{enabled && (
<View style={[sizeStyles, !hasError && styles.alignItemsCenter, styles.justifyContentCenter]}>
{enabled && !hasError && (
Copy link
Contributor

Choose a reason for hiding this comment

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

Why conditional alignItemsCenter styling 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.

The component is used in other places as well, and if I recall correctly, applying alignItemsCenter by default was causing styling issues in other places.

<Pdf
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any reason you added different conditions when rendering the PDF component and thumbnail?

Why are we checking the hasError in native only and not other platforms?

{enabled && !hasError && (

{enabled && thumbnail}

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 I forgot to add that condition, now updated.

fitPolicy={0}
trustAllCerts={false}
Expand All @@ -22,16 +28,27 @@ function PDFThumbnail({previewSourceURL, style, isAuthTokenRequired = false, ena
singlePage
style={sizeStyles}
onError={(error) => {
if (!('message' in error && typeof error.message === 'string' && error.message.match(/password/i))) {
return;
if (onLoadError) {
onLoadError();
}
if (!onPassword) {
if ('message' in error && typeof error.message === 'string' && error.message.match(/password/i) && onPassword) {
onPassword();
return;
}
onPassword();
setHasError(true);
}}
/>
)}
{hasError && (
<View style={[styles.justifyContentCenter, styles.pdfErrorPlaceholder, styles.alignItemsCenter]}>
<Icon
src={Expensicons.ReceiptSlash}
width={variables.receiptPlaceholderIconWidth}
height={variables.receiptPlaceholderIconHeight}
fill={theme.icon}
/>
</View>
)}
</View>
Copy link
Contributor

Choose a reason for hiding this comment

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

The lines of code below are common in both files; we can create a common file for this.

<View style={[styles.justifyContentCenter, styles.pdfErrorPlaceholder, styles.alignItemsCenter]}>
    <Icon
        src={Expensicons.ReceiptSlash}
        width={variables.receiptPlaceholderIconWidth}
        height={variables.receiptPlaceholderIconHeight}
        fill={theme.icon}
    />
</View>

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.

</View>
);
Expand Down
38 changes: 33 additions & 5 deletions src/components/PDFThumbnail/index.tsx
Original file line number Diff line number Diff line change
@@ -1,19 +1,25 @@
// @ts-expect-error - This line imports a module from 'pdfjs-dist' package which lacks TypeScript typings.
import pdfWorkerSource from 'pdfjs-dist/legacy/build/pdf.worker';
import React, {useMemo} from 'react';
import React, {useMemo, useState} from 'react';
import {View} from 'react-native';
import {Document, pdfjs, Thumbnail} from 'react-pdf';
import FullScreenLoadingIndicator from '@components/FullscreenLoadingIndicator';
import Icon from '@components/Icon';
import * as Expensicons from '@components/Icon/Expensicons';
import useTheme from '@hooks/useTheme';
import useThemeStyles from '@hooks/useThemeStyles';
import addEncryptedAuthTokenToURL from '@libs/addEncryptedAuthTokenToURL';
import variables from '@styles/variables';
import type PDFThumbnailProps from './types';

if (!pdfjs.GlobalWorkerOptions.workerSrc) {
pdfjs.GlobalWorkerOptions.workerSrc = URL.createObjectURL(new Blob([pdfWorkerSource], {type: 'text/javascript'}));
}

function PDFThumbnail({previewSourceURL, style, isAuthTokenRequired = false, enabled = true, onPassword}: PDFThumbnailProps) {
function PDFThumbnail({previewSourceURL, style, isAuthTokenRequired = false, enabled = true, onPassword, onLoadError}: PDFThumbnailProps) {
const styles = useThemeStyles();
const theme = useTheme();
const [hasError, setHasError] = useState(false);

const thumbnail = useMemo(
() => (
Expand All @@ -26,18 +32,40 @@ function PDFThumbnail({previewSourceURL, style, isAuthTokenRequired = false, ena
}}
externalLinkTarget="_blank"
onPassword={onPassword}
onLoad={() => {
setHasError(false);
}}
onLoadError={() => {
if (onLoadError) {
onLoadError();
}
setHasError(true);
}}
error={() => null}
>
<View pointerEvents="none">
<Thumbnail pageIndex={0} />
</View>
</Document>
),
[isAuthTokenRequired, previewSourceURL, onPassword],
[isAuthTokenRequired, previewSourceURL, onPassword, onLoadError],
);

return (
<View style={[style, styles.overflowHidden]}>
<View style={[styles.w100, styles.h100, styles.alignItemsCenter, styles.justifyContentCenter]}>{enabled && thumbnail}</View>
<View style={[style, styles.h100, styles.overflowHidden]}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Here, we Added the styles.h100 parent View Component; why is this change necessary? It was not to the parent View Component before our changes. We don't want to add any visual regression from the PR, so we wanted to make sure it is needed 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 found a bug when adding styles.h100 directly, added a condition now.

<View style={[styles.w100, styles.h100, !hasError && styles.alignItemsCenter, !hasError && styles.justifyContentCenter]}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are conditional alignItemsCenter and justifyContentCenter styling passed here?

Instead of checking hasError twice for both stylings, please merge them.

- !hasError && styles.alignItemsCenter, !hasError && styles.justifyContentCenter
+ !hasError && {...styles.alignItemsCenter, ...styles.justifyContentCenter}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated.

{enabled && thumbnail}
{hasError && (
<View style={[styles.justifyContentCenter, styles.pdfErrorPlaceholder, styles.alignItemsCenter]}>
<Icon
src={Expensicons.ReceiptSlash}
width={variables.receiptPlaceholderIconWidth}
height={variables.receiptPlaceholderIconHeight}
fill={theme.icon}
/>
</View>
)}
</View>
</View>
);
}
Expand Down
3 changes: 3 additions & 0 deletions src/components/PDFThumbnail/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@ type PDFThumbnailProps = {

/** Callback to call if PDF is password protected */
onPassword?: () => void;

/** Callback to call if PDF can't be loaded(corrupted) */
onLoadError?: () => void;
};

export default PDFThumbnailProps;
2 changes: 1 addition & 1 deletion src/languages/en.ts
Original file line number Diff line number Diff line change
Expand Up @@ -349,7 +349,7 @@ export default {
expensifyDoesntHaveAccessToCamera: "Expensify can't take photos without access to your camera. Tap Settings to update permissions.",
attachmentError: 'Attachment error',
errorWhileSelectingAttachment: 'An error occurred while selecting an attachment, please try again.',
errorWhileSelectingCorruptedImage: 'An error occurred while selecting a corrupted attachment, please try another file.',
errorWhileSelectingCorruptedAttachment: 'An error occurred while selecting a corrupted attachment, please try another file.',
takePhoto: 'Take photo',
chooseFromGallery: 'Choose from gallery',
chooseDocument: 'Choose document',
Expand Down
2 changes: 1 addition & 1 deletion src/languages/es.ts
Original file line number Diff line number Diff line change
Expand Up @@ -343,7 +343,7 @@ export default {
expensifyDoesntHaveAccessToCamera: 'Expensify no puede tomar fotos sin acceso a la cámara. Haz click en Configuración para actualizar los permisos.',
attachmentError: 'Error al adjuntar archivo',
errorWhileSelectingAttachment: 'Ha ocurrido un error al seleccionar un archivo adjunto. Por favor, inténtalo de nuevo.',
errorWhileSelectingCorruptedImage: 'Ha ocurrido un error al seleccionar un archivo adjunto corrupto. Por favor, inténtalo con otro archivo.',
errorWhileSelectingCorruptedAttachment: 'Ha ocurrido un error al seleccionar un archivo adjunto corrupto. Por favor, inténtalo con otro archivo.',
takePhoto: 'Hacer una foto',
chooseFromGallery: 'Elegir de la galería',
chooseDocument: 'Elegir documento',
Expand Down
2 changes: 1 addition & 1 deletion src/pages/iou/request/step/IOURequestStepScan/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,7 @@ function IOURequestStepScan({
return true;
})
.catch(() => {
setUploadReceiptError(true, 'attachmentPicker.attachmentError', 'attachmentPicker.errorWhileSelectingCorruptedImage');
setUploadReceiptError(true, 'attachmentPicker.attachmentError', 'attachmentPicker.errorWhileSelectingCorruptedAttachment');
return false;
});
}
Expand Down
11 changes: 11 additions & 0 deletions src/styles/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4216,6 +4216,7 @@ const styles = (theme: ThemeColors) =>
borderRadius: 16,
margin: 20,
overflow: 'hidden',
justifyContent: 'center',
},

reportPreviewBox: {
Expand Down Expand Up @@ -4374,6 +4375,16 @@ const styles = (theme: ThemeColors) =>
maxWidth: 400,
},

pdfErrorPlaceholder: {
overflow: 'hidden',
borderWidth: 2,
borderColor: theme.cardBG,
borderRadius: variables.componentBorderRadiusLarge,
maxWidth: 400,
height: '100%',
backgroundColor: theme.highlightBG,
},

moneyRequestAttachReceipt: {
backgroundColor: theme.highlightBG,
borderColor: theme.border,
Expand Down
2 changes: 2 additions & 0 deletions src/styles/variables.ts
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,8 @@ export default {
eReceiptBGHeight: 540,
eReceiptBGHWidth: 335,
eReceiptTextContainerWidth: 263,
receiptPlaceholderIconWidth: 80,
receiptPlaceholderIconHeight: 80,
reportPreviewMaxWidth: 335,
reportActionImagesSingleImageHeight: 147,
reportActionImagesDoubleImageHeight: 138,
Expand Down
Loading