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

fix: mWeb - Chat - Uploading an image while offline briefly shows an offline message in the preview. #43249

Merged
merged 3 commits into from
Jun 19, 2024
Merged

fix: mWeb - Chat - Uploading an image while offline briefly shows an offline message in the preview. #43249

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
27809d6
fix: mWeb - Chat - Uploading an image while offline briefly shows an …
Krishna2323 Jun 7, 2024
02edb5f
Merge branch 'Expensify:main' into krishna2323/issue/42657
Krishna2323 Jun 18, 2024
1047f73
add util function for checking local files.
Krishna2323 Jun 18, 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
10 changes: 6 additions & 4 deletions src/components/ImageView/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,8 @@ function ImageView({isAuthTokenRequired = false, url, fileName, onError}: ImageV
};
}, [canUseTouchScreen, trackMovement, trackPointerPosition]);

const isLocalFile = url.startsWith('blob:') || url.startsWith('file:');
Copy link
Contributor

@allgandalf allgandalf Jun 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Do you think the above two cases cover every type?

For example, here's one example from IOURequestStepConfirmation.tsx, they have extra type check and / check, is the same applicable here?

App/src/pages/iou/request/step/IOURequestStepConfirmation.tsx

Line 206 in a67be2f

const isLocalFile = typeof receiptPath === 'number' || receiptPath?.startsWith('blob:') || receiptPath?.startsWith('file:') || receiptPath?.startsWith('/');

Note

I haven't reviewed the code from laptop cause i'm a little under the weather, so might take the day off, but if you address the above comments of whether those additional checks are necessary, i will try to review the PR over the weekend.

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'm not completely sure about the other checks, with the current changes it works fine on all platforms but we can consider adding those checks also, WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

i think the typeof check would unnecessary here because it will always be true i checked the type ofurl it's string so then isLocalFile would always be true so no need to be redundant.

But startsWith('/') is something i'm not sure about, can you check and see if that is needed? if not then the current implementation is complete, and i will complete the checklist then, let me know :)

Copy link
Contributor

Choose a reason for hiding this comment

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

@Krishna2323 bump on the comments above :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will test for / again on all platforms today

Copy link
Contributor

Choose a reason for hiding this comment

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

bump @Krishna2323 , lets get this merged before the week closes 💪

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move these checks to a shared helper function? @Krishna2323

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@arosiclair, I've updated the code to add a util function for determining local files. @allgandalf, could you please take a look at the updated code? Thanks.


if (canUseTouchScreen) {
return (
<View
Expand All @@ -213,8 +215,8 @@ function ImageView({isAuthTokenRequired = false, url, fileName, onError}: ImageV
onLoad={imageLoad}
onError={onError}
/>
{((isLoading && !isOffline) || (!isLoading && zoomScale === 0)) && <FullscreenLoadingIndicator style={[styles.opacity1, styles.bgTransparent]} />}
{isLoading && <AttachmentOfflineIndicator />}
{((isLoading && (!isOffline || isLocalFile)) || (!isLoading && zoomScale === 0)) && <FullscreenLoadingIndicator style={[styles.opacity1, styles.bgTransparent]} />}
{isLoading && !isLocalFile && <AttachmentOfflineIndicator />}
</View>
);
}
Expand Down Expand Up @@ -247,8 +249,8 @@ function ImageView({isAuthTokenRequired = false, url, fileName, onError}: ImageV
/>
</PressableWithoutFeedback>

{isLoading && !isOffline && <FullscreenLoadingIndicator style={[styles.opacity1, styles.bgTransparent]} />}
{isLoading && <AttachmentOfflineIndicator />}
{isLoading && (!isOffline || isLocalFile) && <FullscreenLoadingIndicator style={[styles.opacity1, styles.bgTransparent]} />}
{isLoading && !isLocalFile && <AttachmentOfflineIndicator />}
</View>
);
}
Expand Down
6 changes: 4 additions & 2 deletions src/components/Lightbox/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,8 @@ function Lightbox({isAuthTokenRequired = false, uri, onScaleChanged: onScaleChan
[onScaleChangedContext, onScaleChangedProp],
);

const isLocalFile = uri.startsWith('blob:') || uri.startsWith('file:');

return (
<View
style={[StyleSheet.absoluteFill, style]}
Expand Down Expand Up @@ -248,13 +250,13 @@ function Lightbox({isAuthTokenRequired = false, uri, onScaleChanged: onScaleChan
)}

{/* Show activity indicator while the lightbox is still loading the image. */}
{isLoading && !isOffline && (
{isLoading && (!isOffline || isLocalFile) && (
<ActivityIndicator
size="large"
style={StyleSheet.absoluteFill}
/>
)}
{isLoading && <AttachmentOfflineIndicator />}
{isLoading && !isLocalFile && <AttachmentOfflineIndicator />}
</>
)}
</View>
Expand Down
Loading