-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Load pdfjs webworker script from source #13546
Conversation
Hey, I noticed you changed some webpack configuration files. This can break production builds. Did you remember to run a production build locally to verify they still work? |
This is ready for review now. While QA-ing this I discovered that there is an existing issue on staging/production where if someone goes Offline the "Send" button disappears on the "Send attachment" screen. This is an Android only issue. I'll try to figure it out tomorrow - if not we can report a new issue. |
@aimane-chnaif @danieldoglas One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Testing blocked on web, mWeb because of this known issue (403 auth error) |
Bump for reviews |
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.
@aimane-chnaif I don't think either of those are blockers for this issue and we can continue with the checklist + testing |
Reviewer Checklist
Screenshots/VideosMobile Web - Chromemchrome.mp4Mobile Web - Safarimsafari.mp4iOSios.mp4Androidandroid.mp4 |
Tests well on all platforms. |
@marcaaron can we ignore this warning? |
In dev, shows this console warning: In prod build, shows this: |
Hmm I'm not really sure what that is about or how to get rid of it - if it doesn't affect the app I'm not sure we need to prioritize it. |
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.
LGTM
I don't think we need to prioritize that error as well. |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Performance Comparison Report 📊Significant Changes To DurationThere are no entries Meaningless Changes To DurationShow entries
Show details
|
🚀 Deployed to staging by @danieldoglas in version: 1.2.41-0 🚀
|
🚀 Deployed to staging by @danieldoglas in version: 1.2.41-0 🚀
|
I think this PR caused regression - #13665 |
🚀 Deployed to production by @yuwenmemon in version: 1.2.41-4 🚀
|
I don't think this was deployed to production because we reverted it... |
Details
When someone goes offline before opening the attachment modal they are unable to load the web workers script via HTTPS. This fix allows loading the worker source as a string in the bundle so it can be used later even if we are offline.
Fixed Issues
$ #12512
Tests
Same as QA.
Note: Use a VALID PDF i.e. something that normally works fine while we are online.
I would recommend sending yourself a valid PDF to an iOS and Android device + hosting the local web version via NGROK then test selecting the attachment with the internet disabled (i.e. Wi-Fi disabled).
Offline tests
This is related to offline behavior so QA is the same.
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
filesWaiting 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)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.Screenshots/Videos
Web
Mobile Web - Chrome
Works. But looks messed up atm and is missing the submit button.
Mobile Web - Safari
Desktop
iOS
N/A as this platform is not touched in this PR
Android
N/A as this platform is not touched in this PR