-
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
Add back qr code download feature #49595
Conversation
I will review it later today. |
On native iOS everything works just fine: iOS: Nativeios._native.movHowever, on Android, I am unable to download. When I click the download button, nothing happens. I will check if the issue is related to my emulator and report back. Android: Nativeandroid_native.movAlso on other platforms I can't see the button at all: iOS: mWeb Safariios_web.movMacOS: Chrome / Safariweb.movMacOS: Desktopdesktop.mov@lakchote Is this expected? Thanks. |
Yes, that's normal @brunovjk. We only show this option on native platforms.
I'm going to test this out. |
@brunovjk I've updated code to include another area where we should add back the QR code's download feature. Android doesn't seem to work either, I'm looking. |
@WoLewicki I've stumbled upon your comment here. Bumping RN version to 0.75 #37374 seemed to have make the QR code download works on iOS. But on Android, the problem remains. Would you mind trying again if the patch works? We're still using the old version of react-native-view-shot, which is compatible with the old arch. |
You need to upgrade to the latest |
Thanks, I've used version @brunovjk could you please retest? |
Reviewer Checklist
Screenshots/VideosAndroid: Native49595_android_native.movAndroid: mWeb Chrome49595_android_web.moviOS: Native49595_ios_native.moviOS: mWeb SafariMacOS: Chrome / Safari49595_web_chorme.movMacOS: Desktop49595_web_desktop.mov |
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 :D
@carlosmiceli Please 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] |
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.
We only show this option on native platforms.
Why is this?
For this reason |
Odd but fair enough. Not sure why I was subscribed to this PR 😄 |
@@ -82,6 +84,8 @@ function ShareCodePage({report, policy}: ShareCodePageProps) { | |||
}, [report, currentUserPersonalDetails, isReport]); | |||
|
|||
const title = isReport ? ReportUtils.getReportName(report) : currentUserPersonalDetails.displayName ?? ''; | |||
const platform = getPlatform(); | |||
const isNative = platform === CONST.PLATFORM.IOS || platform === CONST.PLATFORM.ANDROID; |
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.
If this hasn't been discussed previously, why didn't we follow the no specific platform conditions rule?
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.
I don't have the answer to that question. It is preexisting code, I think that's @robertjchen that did this feature.
I just didn't want to introduce potential side effects that may make this feature broken, since this PR aims to put back code that was present before.
(You've subscribed to this PR since I've chatted with you yesterday about react-native-view-shot
)
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.
Oh right lol, had already forgotten
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.
I know its annoying, but if we do ship the isMobile condition we should state the reason why. The more we have, the more people will use it as justification to introduce more.
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.
Ah, this was from a while back- IIRC we added this hack due to limitations with the QR library, in order to get the feature working in time for a conference. 🤔
Since there's no time pressure anymore- maybe we can look into fixing this by seeing if the limitation still exists, and if we still need this check?
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.
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.
cc @Julesssss @robertjchen I think we still need to keep the platform check for now.
We'll remove the condition once #19834 is done.
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.
:sadcat_okay: Yep, that would be the proper issue to handle this.
Could we make a big comment there noting that this is a bad thing and that it'll be addressed in #19834 🙏
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.
Fair enough, thanks for investigating
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.
I've added comments to make sure we don't follow the same logic elsewhere in our codebase.
Thank you both @Julesssss @robertjchen for checking!
Sorry, back from OOO, will wait for response to @Julesssss question above and review! |
✋ 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/lakchote in version: 9.0.41-0 🚀
|
Looks like there are some cases where this code doesn't work as expected: #49868. Weird because it looks like it worked in the screenshots of this PR. Maybe it was a difference between debug and release builds? |
Yes, it did work both for the C+ and myself. Also I've just retested on |
🚀 Deployed to production by https://github.com/jasperhuangg in version: 9.0.41-10 🚀
|
Details
QR code download feature was removed since it wasn't compatible with the new arch (Fabric #13767) when it got merged.
We can add it again since #37374 got merged.
Fixed Issues
$ #40110
PROPOSAL:
Tests
Test 1: workspace's settings
Profile
Share
Download
Test 2: report's details
Share
Download
QA Steps
Same as in 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./** 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
)Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
Screen.Recording.2024-09-23.at.13.56.58.mov
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop