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

Add back the QR code download button #40110

Open
lakchote opened this issue Apr 11, 2024 · 15 comments
Open

Add back the QR code download button #40110

lakchote opened this issue Apr 11, 2024 · 15 comments
Assignees
Labels
Internal Requires API changes or must be handled by Expensify staff Monthly KSv2

Comments

@lakchote
Copy link
Contributor

lakchote commented Apr 11, 2024

QR code download doesn't work anymore (as of today) since the new arch (Fabric PR) got merged.

TL;DR: react-native-view-shot does not support new arch yet.

It was decided here to hide the Download button for QR code in the meantime, to prevent users from encountering bugs and degrading user experience.

When the update 0.74 for react-native is available, we should:

  • use it so react-native-view-shot works again as intended
  • Revert PR here and show back the Download button.
Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01d1641a383b71483e
  • Upwork Job ID: 1778781286743756800
  • Last Price Increase: 2024-04-12
@lakchote lakchote added Daily KSv2 Weekly KSv2 labels Apr 11, 2024
@lakchote lakchote self-assigned this Apr 11, 2024
@lakchote lakchote removed the Daily KSv2 label Apr 12, 2024
@ShridharGoel
Copy link
Contributor

Proposal

Please re-state the problem that we are trying to solve in this issue.

QR code download using react-native-view-shot doesn't work after moving to the new arch.

What is the root cause of that problem?

The react-native-view-shot library has introduced the new architecture support in latest version, but we are still using 3.8.0. (gre/react-native-view-shot#516)

Also, these changes in react-native need to be included.

What changes do you think we should make in order to solve the problem?

Add patch for the view-shot library with the changes for the new architecture or bump the version to 4.0.0-alpha.0.
Add patch for these changes in react-native.

Note: This is same as my proposal here.

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Apr 12, 2024
@lakchote
Copy link
Contributor Author

@ShridharGoel thanks for your proposal but please refrain posting proposals in issues where the Help wanted label is not present.

Instead, post (like you did) in the issue where proposals are allowed, thank you!

@lakchote lakchote added the Internal Requires API changes or must be handled by Expensify staff label Apr 12, 2024
Copy link

melvin-bot bot commented Apr 12, 2024

Job added to Upwork: https://www.upwork.com/jobs/~01d1641a383b71483e

@lakchote lakchote added Weekly KSv2 and removed Daily KSv2 labels Apr 12, 2024
Copy link

melvin-bot bot commented Apr 12, 2024

Triggered auto assignment to Contributor Plus for review of internal employee PR - @ahmedGaber93 (Internal)

@lakchote
Copy link
Contributor Author

Still waiting for RN 0.74 to be effective. Tracking PR here.

@melvin-bot melvin-bot bot removed the Overdue label Apr 29, 2024
@melvin-bot melvin-bot bot added the Overdue label May 7, 2024
@lakchote
Copy link
Contributor Author

lakchote commented May 8, 2024

Still waiting for RN 0.74 to be effective. Tracking PR here.

Still waiting for RN 0.74 PR to be available. Putting this as a monthly in the meantime.

@melvin-bot melvin-bot bot removed the Overdue label May 8, 2024
@lakchote lakchote added Monthly KSv2 and removed Weekly KSv2 labels May 8, 2024
@melvin-bot melvin-bot bot added the Overdue label Jun 10, 2024
@ahmedGaber93
Copy link
Contributor

Not overdue, Still hold on #37374

@melvin-bot melvin-bot bot removed the Overdue label Jun 10, 2024
@melvin-bot melvin-bot bot added the Overdue label Jul 11, 2024
@lakchote
Copy link
Contributor Author

Not overdue, Still hold on #37374

Same.

@melvin-bot melvin-bot bot removed the Overdue label Jul 12, 2024
@melvin-bot melvin-bot bot added the Overdue label Aug 12, 2024
@ahmedGaber93
Copy link
Contributor

Not overdue, Still hold on #37374

@melvin-bot melvin-bot bot removed the Overdue label Aug 13, 2024
@ahmedGaber93
Copy link
Contributor

I will unassign me to clean up my k2.

@ahmedGaber93 ahmedGaber93 removed their assignment Aug 13, 2024
@s77rt
Copy link
Contributor

s77rt commented Sep 12, 2024

Alternative solution: @s77rt/react-native-viewshot

@melvin-bot melvin-bot bot added the Overdue label Sep 13, 2024
@lakchote
Copy link
Contributor Author

PR #37374 was merged, will take a look to add it back as soon as I can.

@melvin-bot melvin-bot bot removed the Overdue label Sep 19, 2024
@lakchote lakchote added the Reviewing Has a PR in review label Sep 23, 2024
@melvin-bot melvin-bot bot added Weekly KSv2 and removed Monthly KSv2 labels Sep 23, 2024
Copy link

melvin-bot bot commented Sep 28, 2024

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

@lakchote lakchote added Monthly KSv2 and removed Reviewing Has a PR in review Weekly KSv2 labels Sep 30, 2024
@lakchote
Copy link
Contributor Author

PR didn't solve the issue, see #49868 (comment). We're going to wait for a new stable version.

@brunovjk
Copy link
Contributor

brunovjk commented Oct 2, 2024

I don't have a physical iOS device, I only have Android. I'm available to test if necessary. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Internal Requires API changes or must be handled by Expensify staff Monthly KSv2
Projects
None yet
Development

No branches or pull requests

5 participants