-
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
feat: add PDFThumbnail to preview PDF receipt #35255
feat: add PDFThumbnail to preview PDF receipt #35255
Conversation
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.
Please fix typescript errors
a381041
to
58282ca
Compare
Hmm, I'm still unable to build iOS App after rebasing on the main branch. I got following error when run [!] The following Swift pods cannot yet be integrated as static libraries:
The Swift pod `ExpoModulesCore` depends upon `glog`, which does not define modules. To opt into those targets generating module maps (which is necessary to import them from Swift when building as static libraries), you may set `use_modular_headers!` globally in your Podfile, or specify `:modular_headers => true` for particular dependencies. Maybe it'll be solved after #35249 is merged? |
On Web the pdf preview have a cursor pointer, let's use the default pointer instead as the preview is not clickable Screen.Recording.2024-01-28.at.7.51.58.PM.mov |
The local preview do not match the one returned by the server Screen.Recording.2024-01-28.at.9.31.20.PM.movScreen.Recording.2024-01-28.at.8.15.55.PM.mov |
Uploading a password protected pdf results in a non-ending prompt unless you type the right pasword Screen.Recording.2024-01-28.at.9.14.00.PM.mov |
Oh yes, I also noticed this but it seems hard to implement the same with what backend does. See #31432 (comment) about the backend implementation. Should we ask the product team to decide if we need to keep the resolution of local PDF thumbnail same as the one returned by backend? |
be0f6e4
to
b000ed8
Compare
@eh2077 Please check #35255 (comment) For the password bug, I think we should show an error if the user is uploading a password protected file since the BE can't scan it anyway |
Got it and I'll update later |
@eh2077 Any updates here? |
@s77rt My updates are as follows For thumbnail resolution/size not consistent issue, I tried to tune the size of PDF by passing Kindly let me know your thoughts on them. |
@eh2077 Regarding the password issue, previously the uploaded file would just fail but now it's creating an endless prompt which is something that we should fix. The least we can do is to prevent this prompt and let the uploaded file just fail as on |
@eh2077 Any updates here? |
@s77rt Sorry for the delay. I'm working on fixing the endless prompt. I'll push an update early tmr. |
b000ed8
to
7c2425d
Compare
@s77rt I did a forced push in this PR to solve conflicts and avoid missing changes from main branch. I fixed the endless password prompt issue of web platform (native platforms don't have this issue) by passing an empty Screen.Recording.2024-02-13.at.4.20.38.PM.movIf the pdf uploaded is protected, it shows the |
@eh2077 Please do not force push. This messes up the review process. Can you try undo that force push? |
That may confuse the user, can we show instead the generic pdf thumbnail? |
@s77rt The pointer issue is fixed. |
For thumbnail resolution/size not consistent issue, I tried to tune the size of PDF by passing width or height property to the PDF thumbnail component. But it seems not easy to achieve what we want in this way. The PDF is rendered as |
I don't want to hold getting thumbnails of PDFs out on that for sure. I think it's also being looked into here tbh: #35041 (comment) |
@eh2077 In tests steps change |
@eh2077 Thank you! |
Just one small translation change |
Co-authored-by: Carlos Martins <luacmartins@gmail.com>
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/luacmartins in version: 1.4.48-0 🚀
|
I think this might have caused this regression. |
🚀 Deployed to production by https://github.com/roryabraham in version: 1.4.48-0 🚀
|
const thumbnail = useMemo( | ||
() => ( | ||
<Document | ||
loading={<FullScreenLoadingIndicator />} |
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.
Coming for this issue #39356 , since we are not passing the error prop to the Document, the Document will display the message 'Failed to load PDF file.' text in case of an error. This, however, causes a UI issue in the dark theme of our app. we should have passed a custom error component here to avoid UI issue.
Details
Fixed Issues
$ #31432
PROPOSAL: #31432 (comment)
Tests
Request
button to send money requestOffline tests
QA Steps
Same as tests
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
0-android-offline.mp4
0-android-online.mp4
Android: mWeb Chrome
0-mobile-chrome-offline.mp4
0-mobile-chrome-online.mp4
iOS: Native
0-ios-offline.mp4
0-ios-online.mp4
iOS: mWeb Safari
0-mobile-safari-offline.mp4
0-mobile-safari-online.mp4
MacOS: Chrome / Safari
0-web-offline.mp4
0-web-online.mp4
MacOS: Desktop
0-desktop-offline.mp4
0-desktop-online.mp4