-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Changes from 13 commits
df364c5
1b4bf57
1e046d8
2288b18
24577f1
f258c08
dbc3286
d6a166b
8745bc8
bbcea3c
eb01458
2eeadec
31caba2
601eb77
075868d
dc778d7
1f93a1e
16cb64f
364acd6
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 | ||||
---|---|---|---|---|---|---|
@@ -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); | ||||||
|
||||||
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.
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.
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. 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 && ( | ||||||
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. Why conditional 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. The component is used in other places as well, and if I recall correctly, applying |
||||||
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. 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?
App/src/components/PDFThumbnail/index.tsx Line 57 in 31caba2
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. I think I forgot to add that condition, now updated. |
||||||
fitPolicy={0} | ||||||
trustAllCerts={false} | ||||||
|
@@ -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> | ||||||
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. 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> 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. |
||||||
</View> | ||||||
); | ||||||
|
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( | ||
() => ( | ||
|
@@ -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]}> | ||
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. Here, we Added the 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. I found a bug when adding |
||
<View style={[styles.w100, styles.h100, !hasError && styles.alignItemsCenter, !hasError && styles.justifyContentCenter]}> | ||
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. Why are conditional Instead of checking hasError twice for both stylings, please merge them. - !hasError && styles.alignItemsCenter, !hasError && styles.justifyContentCenter
+ !hasError && {...styles.alignItemsCenter, ...styles.justifyContentCenter} 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. 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> | ||
); | ||
} | ||
|
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.
NAB: If we replace the wording from Image to Attachment for the translate key, should we change the function name to
showAttachmentCorruptionAlert
?