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 blank area after loading pdf #38010

Merged
merged 60 commits into from
May 15, 2024
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
60 commits
Select commit Hold shift + click to select a range
ffb21f1
add POC for fallingback to default attachment view when pdf fails to …
ishpaul777 Mar 8, 2024
bde48fa
adds chnages for attachement view
ishpaul777 Mar 8, 2024
9323594
Merge branch 'fix/blankArea-for-corruptedPDF' into fix-blankArea-afte…
ishpaul777 Mar 8, 2024
757661b
formatting
ishpaul777 Mar 8, 2024
c4fce87
Merge branch 'fix-blankArea-after-loading-pdf' of https://github.com/…
ishpaul777 Mar 8, 2024
26c1f13
fixes conflicts
ishpaul777 Mar 8, 2024
36b5814
fix conflict after merge
ishpaul777 Mar 8, 2024
f7b6cd5
Merge branch 'Expensify:main' into fix-blankArea-after-loading-pdf
ishpaul777 Mar 13, 2024
75a733c
clean up and lint fix
ishpaul777 Mar 13, 2024
1a84b64
Merge branch 'Expensify:main' into fix-blankArea-after-loading-pdf
ishpaul777 Mar 15, 2024
e2b9966
add POC for fallingback to default attachment view when pdf fails to …
ishpaul777 Mar 8, 2024
02f26eb
fixes missing prop
ishpaul777 Mar 15, 2024
2d37945
Merge branch 'fix-blankArea-after-loading-pdf' of https://github.com/…
ishpaul777 Mar 15, 2024
b4ac9a1
remove unused import
ishpaul777 Mar 15, 2024
0372601
change variable name
ishpaul777 Mar 15, 2024
a20811a
fixes lint
ishpaul777 Mar 15, 2024
3ace4d3
use correct conditional and use js docs for props
ishpaul777 Mar 15, 2024
830a392
prettier diffs
ishpaul777 Mar 15, 2024
3813026
Update src/components/Attachments/AttachmentCarousel/CarouselItem.js
ishpaul777 Mar 15, 2024
a8cfafe
remove unused styles
ishpaul777 Mar 15, 2024
5199de9
Merge branch 'Expensify:main' into fix-blankArea-after-loading-pdf
ishpaul777 Mar 16, 2024
ccbcfe0
Merge branch 'Expensify:main' into fix-blankArea-after-loading-pdf
ishpaul777 Mar 19, 2024
a45f6f0
remove error label styles
ishpaul777 Mar 19, 2024
1f489cf
Merge branch 'fix-blankArea-after-loading-pdf' of https://github.com/…
ishpaul777 Mar 19, 2024
cb7ba78
Merge branch 'main' into fix-blankArea-after-loading-pdf
ishpaul777 Mar 20, 2024
81efdb2
Merge branch 'main' into fix-blankArea-after-loading-pdf
ishpaul777 Mar 28, 2024
32f3503
fix conflicts
ishpaul777 Mar 28, 2024
1c814c5
fix lint
ishpaul777 Mar 28, 2024
302fdba
fallback to defaultview component
ishpaul777 Mar 28, 2024
3b51760
fix prop pass
ishpaul777 Mar 28, 2024
7d5d22a
prettier diffs
ishpaul777 Mar 28, 2024
088813d
Merge branch 'Expensify:main' into fix-blankArea-after-loading-pdf
ishpaul777 Mar 29, 2024
0af56c6
remove renderfallbackview prop
ishpaul777 Mar 29, 2024
b18b06a
fix the comment for the prop
ishpaul777 Mar 29, 2024
6436ddb
fixes loading for pdf on web
ishpaul777 Mar 29, 2024
359c54d
fixes proptype warning
ishpaul777 Mar 29, 2024
691e2a8
prettier diffs
ishpaul777 Mar 29, 2024
2b1bff7
Merge branch 'Expensify:main' into fix-blankArea-after-loading-pdf
ishpaul777 Apr 2, 2024
9bd4c96
adds onError prop in react-fast-pdf
ishpaul777 Apr 2, 2024
7003564
Merge branch 'main' into fix-blankArea-after-loading-pdf
ishpaul777 Apr 6, 2024
e714b2f
resolve conflicts
ishpaul777 Apr 6, 2024
15eea96
code formatting
ishpaul777 Apr 6, 2024
7a9565a
remove unnecessary change
ishpaul777 Apr 6, 2024
6912979
fix type checks
ishpaul777 Apr 6, 2024
917095c
lint code
ishpaul777 Apr 6, 2024
c968147
prettier diffs
ishpaul777 Apr 6, 2024
cadf1f1
resolve lint failure
ishpaul777 Apr 6, 2024
54eb295
prettier diffs
ishpaul777 Apr 6, 2024
3044ee9
Merge branch 'Expensify:main' into fix-blankArea-after-loading-pdf
ishpaul777 Apr 11, 2024
9c1527b
remove unnecessary styles
ishpaul777 Apr 11, 2024
3c871f1
Merge branch 'Expensify:main' into fix-blankArea-after-loading-pdf
ishpaul777 Apr 16, 2024
0338f8d
merge main
ishpaul777 May 3, 2024
838e465
prettier diffs
ishpaul777 May 3, 2024
17ed872
Merge branch 'Expensify:main' into fix-blankArea-after-loading-pdf
ishpaul777 May 13, 2024
3763d32
fixes preview not centered in carousel
ishpaul777 May 13, 2024
da41661
fix loading component
ishpaul777 May 13, 2024
c134239
prettier
ishpaul777 May 13, 2024
9c1b2c4
Merge branch 'Expensify:main' into fix-blankArea-after-loading-pdf
ishpaul777 May 15, 2024
e1d69f6
Add-empty-newlines
ishpaul777 May 15, 2024
569b7c1
adds requested changes
ishpaul777 May 15, 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
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ function BaseAnchorForAttachmentsOnly({style, source = '', displayName = '', dow
file={{name: displayName}}
shouldShowDownloadIcon={!isOffline}
shouldShowLoadingSpinnerIcon={isDownloading}
isUsedAsChatAttachment
/>
</PressableWithoutFeedback>
)}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ function CarouselItem({item, onPress, isFocused, isModalHovered}) {
isHovered={isModalHovered}
isFocused={isFocused}
optionalVideoDuration={item.duration}
isUsedInCarousel
ishpaul777 marked this conversation as resolved.
Show resolved Hide resolved
/>
</View>

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ function BaseAttachmentViewPdf({
onLoadComplete,
errorLabelStyles,
style,
isUsedAsChatAttachment,
onError,
}) {
const attachmentCarouselPagerContext = useContext(AttachmentCarouselPagerContext);
const isScrollEnabled = attachmentCarouselPagerContext === null ? undefined : attachmentCarouselPagerContext.isScrollEnabled;
Expand Down Expand Up @@ -89,6 +91,8 @@ function BaseAttachmentViewPdf({
onScaleChanged={onScaleChanged}
onLoadComplete={onLoadComplete}
errorLabelStyles={errorLabelStyles}
isUsedAsChatAttachment={isUsedAsChatAttachment}
onError={onError}
/>
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import React, {memo} from 'react';
import PDFView from '@components/PDFView';
import {attachmentViewPdfDefaultProps, attachmentViewPdfPropTypes} from './propTypes';

function AttachmentViewPdf({file, encryptedSourceUrl, isFocused, onPress, onToggleKeyboard, onLoadComplete, errorLabelStyles, style}) {
function AttachmentViewPdf({file, encryptedSourceUrl, isFocused, onPress, onToggleKeyboard, onLoadComplete, errorLabelStyles, style, onError, isUsedAsChatAttachment}) {
return (
<PDFView
onPress={onPress}
Expand All @@ -13,6 +13,8 @@ function AttachmentViewPdf({file, encryptedSourceUrl, isFocused, onPress, onTogg
onToggleKeyboard={onToggleKeyboard}
onLoadComplete={onLoadComplete}
errorLabelStyles={errorLabelStyles}
onError={onError}
isUsedAsChatAttachment={isUsedAsChatAttachment}
/>
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,12 @@ const attachmentViewPdfPropTypes = {

/** Styles for the error label */
errorLabelStyles: stylePropTypes,

/** Callback when the pdf fails to load */
onError: PropTypes.func,

/** Whether the attachment is used as a chat attachment */
isUsedAsChatAttachment: PropTypes.bool,
};

const attachmentViewPdfDefaultProps = {
Expand All @@ -23,6 +29,8 @@ const attachmentViewPdfDefaultProps = {
},
style: [],
errorLabelStyles: [],
onError: () => {},
isUsedAsChatAttachment: false,
};

export {attachmentViewPdfPropTypes, attachmentViewPdfDefaultProps};
14 changes: 13 additions & 1 deletion src/components/Attachments/AttachmentView/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -62,9 +62,14 @@ const propTypes = {
/** The id of the report action related to the attachment */
reportActionID: PropTypes.string,

/** Whether the attachment is currently being hovered over */
isHovered: PropTypes.bool,

/** The duration of the video */
optionalVideoDuration: PropTypes.number,

/** Whether the attachment is being used as a chat attachment */
isUsedAsChatAttachment: PropTypes.bool,
};

const defaultProps = {
Expand All @@ -79,6 +84,7 @@ const defaultProps = {
reportActionID: '',
isHovered: false,
optionalVideoDuration: 0,
isUsedAsChatAttachment: false,
};

function AttachmentView({
Expand All @@ -101,13 +107,15 @@ function AttachmentView({
reportActionID,
isHovered,
optionalVideoDuration,
isUsedAsChatAttachment,
}) {
const {updateCurrentlyPlayingURL} = usePlaybackContext();
const theme = useTheme();
const styles = useThemeStyles();
const StyleUtils = useStyleUtils();
const [loadComplete, setLoadComplete] = useState(false);
const isVideo = (typeof source === 'string' && Str.isVideo(source)) || (file && Str.isVideo(file.name));
const [isPdfFailedToLoad, setIsPdfFailedToLoad] = useState(false);

useEffect(() => {
if (!isFocused && !(file && isUsedInAttachmentModal)) {
Expand Down Expand Up @@ -157,7 +165,7 @@ function AttachmentView({

// Check both source and file.name since PDFs dragged into the text field
// will appear with a source that is a blob
if ((_.isString(source) && Str.isPDF(source)) || (file && Str.isPDF(file.name || translate('attachmentView.unknownFilename')))) {
if (!isPdfFailedToLoad && ((_.isString(source) && Str.isPDF(source)) || (file && Str.isPDF(file.name || translate('attachmentView.unknownFilename'))))) {
const encryptedSourceUrl = isAuthTokenRequired ? addEncryptedAuthTokenToURL(source) : source;

const onPDFLoadComplete = (path) => {
Expand Down Expand Up @@ -187,6 +195,10 @@ function AttachmentView({
errorLabelStyles={isUsedInAttachmentModal ? [styles.textLabel, styles.textLarge] : [styles.cursorAuto]}
style={isUsedInAttachmentModal ? styles.imageModalPDF : styles.flex1}
isUsedInCarousel={isUsedInCarousel}
isUsedAsChatAttachment={isUsedAsChatAttachment}
onError={() => {
setIsPdfFailedToLoad(true);
}}
/>
</View>
);
Expand Down
2 changes: 1 addition & 1 deletion src/components/InvertedFlatList/BaseInvertedFlatList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import type {FlatListProps} from 'react-native';
import FlatList from '@components/FlatList';

const WINDOW_SIZE = 15;
const AUTOSCROLL_TO_TOP_THRESHOLD = 128;
const AUTOSCROLL_TO_TOP_THRESHOLD = 250;

function BaseInvertedFlatList<T>(props: FlatListProps<T>, ref: ForwardedRef<FlatList>) {
return (
Expand Down
13 changes: 4 additions & 9 deletions src/components/PDFView/WebPDFDocument.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,11 @@ import {Document} from 'react-pdf';
import {VariableSizeList as List} from 'react-window';
import _ from 'underscore';
import FullScreenLoadingIndicator from '@components/FullscreenLoadingIndicator';
import Text from '@components/Text';
import stylePropTypes from '@styles/stylePropTypes';
import CONST from '@src/CONST';
import PageRenderer from './WebPDFPageRenderer';

const propTypes = {
/** Index of the PDF page to be displayed passed by VariableSizeList */
errorLabelStyles: stylePropTypes,
/** Returns translated string for given locale and phrase */
translate: PropTypes.func.isRequired,
/** The source URL from which to load PDF file to be displayed */
sourceURL: PropTypes.string.isRequired,
/** Callback invoked when the PDF document is loaded successfully */
Expand Down Expand Up @@ -48,19 +43,18 @@ const propTypes = {
* - `undefined` if password isn't needed to view the PDF file
* - `null` if the password is required but hasn't been provided yet */
password: PropTypes.string,
/** Callback invoked when the PDF document fails to load */
onError: PropTypes.func.isRequired,
};

const defaultProps = {
errorLabelStyles: [],
numPages: null,
listStyle: undefined,
password: undefined,
};

const WebPDFDocument = memo(
({
errorLabelStyles,
ishpaul777 marked this conversation as resolved.
Show resolved Hide resolved
translate,
sourceURL,
onDocumentLoadSuccess,
pageViewportsLength,
Expand All @@ -76,6 +70,7 @@ const WebPDFDocument = memo(
listStyle,
initiatePasswordChallenge,
password,
onError,
}) => {
const onPassword = useCallback(
(callback, reason) => {
Expand All @@ -95,7 +90,7 @@ const WebPDFDocument = memo(
return (
<Document
loading={<FullScreenLoadingIndicator />}
error={<Text style={errorLabelStyles}>{translate('attachmentView.failedToLoadPDF')}</Text>}
onLoadError={onError}
file={sourceURL}
options={{
cMapUrl: 'cmaps/',
Expand Down
1 change: 1 addition & 0 deletions src/components/PDFView/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,7 @@ class PDFView extends Component {
pageWidth={pageWidth}
password={this.state.password}
initiatePasswordChallenge={this.initiatePasswordChallenge}
onError={this.props.onError}
/>
</View>
{(this.state.password === PDFViewConstants.REQUIRED_PASSWORD_MISSING || this.state.isCheckingPassword) && (
Expand Down
25 changes: 14 additions & 11 deletions src/components/PDFView/index.native.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import PDF from 'react-native-pdf';
import FullScreenLoadingIndicator from '@components/FullscreenLoadingIndicator';
import KeyboardAvoidingView from '@components/KeyboardAvoidingView';
import PressableWithoutFeedback from '@components/Pressable/PressableWithoutFeedback';
import Text from '@components/Text';
import useKeyboardState from '@hooks/useKeyboardState';
import useLocalize from '@hooks/useLocalize';
import useStyleUtils from '@hooks/useStyleUtils';
Expand Down Expand Up @@ -33,7 +32,10 @@ const propTypes = {
* is (temporarily) rendered.
*/

function PDFView({onToggleKeyboard, onLoadComplete, fileName, onPress, isFocused, onScaleChanged, sourceURL, errorLabelStyles}) {
const LOADING_THUMBNAIL_HEIGHT = 250;
const LOADING_THUMBNAIL_WIDTH = 250;

function PDFView({onToggleKeyboard, onLoadComplete, fileName, onPress, isFocused, onScaleChanged, sourceURL, onError, isUsedAsChatAttachment}) {
const [shouldRequestPassword, setShouldRequestPassword] = useState(false);
const [shouldAttemptPDFLoad, setShouldAttemptPDFLoad] = useState(true);
const [shouldShowLoadingIndicator, setShouldShowLoadingIndicator] = useState(true);
Expand Down Expand Up @@ -84,6 +86,7 @@ function PDFView({onToggleKeyboard, onLoadComplete, fileName, onPress, isFocused
setShouldShowLoadingIndicator(false);
setShouldRequestPassword(false);
setShouldAttemptPDFLoad(false);
onError(error);
};

/**
Expand Down Expand Up @@ -115,7 +118,9 @@ function PDFView({onToggleKeyboard, onLoadComplete, fileName, onPress, isFocused
};

function renderPDFView() {
const pdfStyles = [themeStyles.imageModalPDF, StyleUtils.getWidthAndHeightStyle(windowWidth, windowHeight)];
const pdfHeight = isUsedAsChatAttachment ? LOADING_THUMBNAIL_HEIGHT : windowHeight;
const pdfWeight = isUsedAsChatAttachment ? LOADING_THUMBNAIL_WIDTH : windowWidth;
const pdfStyles = [StyleUtils.getWidthAndHeightStyle(pdfWeight, pdfHeight), themeStyles.imageModalPDF];

// If we haven't yet successfully validated the password and loaded the PDF,
// then we need to hide the react-native-pdf/PDF component so that PDFPasswordForm
Expand All @@ -124,21 +129,19 @@ function PDFView({onToggleKeyboard, onLoadComplete, fileName, onPress, isFocused
if (shouldRequestPassword) {
pdfStyles.push(themeStyles.invisible);
}

const containerStyles = shouldRequestPassword && isSmallScreenWidth ? [themeStyles.w100, themeStyles.flex1] : [themeStyles.alignItemsCenter, themeStyles.flex1];
const containerStyles =
isUsedAsChatAttachment || (shouldRequestPassword && isSmallScreenWidth) ? [themeStyles.w100, themeStyles.flex1] : [themeStyles.alignItemsCenter, themeStyles.flex1];
const loadingIndicatorStyles = isUsedAsChatAttachment
? [themeStyles.chatItemPDFAttachmentLoading, StyleUtils.getWidthAndHeightStyle(LOADING_THUMBNAIL_WIDTH, LOADING_THUMBNAIL_HEIGHT)]
: [];
Copy link
Member

Choose a reason for hiding this comment

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

Could you please explain these UI changes? Also, are we still loading the PDF invisibly?
I am not very clear on that. Do you mind explaining that part again in detail with visuals?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we ned to have different ui styles for loading in chat vs loading in attachment modal

in chat in modal
Screenshot 2024-03-19 at 1 40 56 PM Screenshot 2024-03-19 at 1 40 32 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

are we still loading the PDF invisibly?

no we aren't anymore


return (
<View style={containerStyles}>
{failedToLoadPDF && (
<View style={[themeStyles.flex1, themeStyles.justifyContentCenter]}>
<Text style={errorLabelStyles}>{translate('attachmentView.failedToLoadPDF')}</Text>
</View>
)}
{shouldAttemptPDFLoad && (
<PDF
fitPolicy={0}
trustAllCerts={false}
renderActivityIndicator={() => <FullScreenLoadingIndicator />}
renderActivityIndicator={() => <FullScreenLoadingIndicator style={loadingIndicatorStyles} />}
source={{uri: sourceURL, cache: true, expiration: 864000}}
style={pdfStyles}
onError={handleFailureToLoadPDF}
Expand Down
14 changes: 14 additions & 0 deletions src/styles/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -516,6 +516,10 @@ const styles = (theme: ThemeColors) =>
borderRadius: variables.buttonBorderRadius,
},

componentBorderRadiusNormal: {
borderRadius: variables.componentBorderRadiusNormal,
},

ishpaul777 marked this conversation as resolved.
Show resolved Hide resolved
bottomTabBarContainer: {
flexDirection: 'row',
height: variables.bottomTabHeight,
Expand Down Expand Up @@ -2120,6 +2124,16 @@ const styles = (theme: ThemeColors) =>
width: 200,
},

chatItemPDFAttachmentLoading: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious why we have to write all new styles here? Don't we have something identical to this that already exists?

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 couldn't find any similar style to this, we could use individual style in components but thats just writing all of these twice (we have different pdf component for native and web)

backgroundColor: 'transparent',
borderColor: theme.border,
borderWidth: 1,
borderRadius: variables.componentBorderRadiusNormal,
textAlign: 'center',
verticalAlign: 'middle',
opacity: 1,
},

sidebarVisible: {
borderRightWidth: 1,
},
Expand Down
Loading