search

Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.33k stars 2.76k forks source link

Migrate changes in `react-native-qrcode-svg-old` to `react-native-qrcode-svg` #47711

Open grgia opened 3 weeks ago

grgia commented 3 weeks ago

See https://expensify.slack.com/archives/C01GTK53T8Q/p1724159312329299?thread_ts=1697098846.087309&cid=C01GTK53T8Q

Background

Goal

melvin-bot[bot] commented 3 weeks ago

Triggered auto assignment to @bfitzexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

grgia commented 3 weeks ago

cc @Kicu - Do you think someone else from SWM would be interested in looking into this? Otherwise I can extend out to one of our other agencies

melvin-bot[bot] commented 3 weeks ago

Current assignee @bfitzexpensify is eligible for the NewFeature assigner, not assigning anyone new.

melvin-bot[bot] commented 3 weeks ago

:warning: It looks like this issue is labelled as a New Feature but not tied to any GitHub Project. Keep in mind that all new features should be tied to GitHub Projects in order to properly track external CAP software time :warning:

melvin-bot[bot] commented 3 weeks ago

Triggered auto assignment to Design team member for new feature review - @shawnborton (NewFeature)

grgia commented 3 weeks ago

Not really a bug not really a new feature... more of an improvement

grgia commented 3 weeks ago

Posted in SWM https://expensify.slack.com/archives/C04878MDF34/p1724176578092919

Kicu commented 3 weeks ago

Because Expensify is now the owner of this open source library I think we should try to keep commits and PRs clean and self contained.

Because of this my suggestion would be to migrate the two PRs that we did on the original fork separately:

If we do that, then later when testing or if anything breaks we can easily revert the svg code, while the example app should stay as it helps with development a lot.

I can migrate the two separately and open them on https://github.com/Expensify/react-native-qrcode-svg/

Kicu commented 3 weeks ago

CC @grgia @luacmartins

luacmartins commented 3 weeks ago

Agree with that. Let's do it.

Kicu commented 3 weeks ago

PR that adds the Example app: https://github.com/Expensify/react-native-qrcode-svg/pull/206 Unsure who is responsible now for reviewing and merging this - is it anyone working in Expensify? I can call on some friends that work on Open Source in SWM to also take a look at the PR if you guys want it.

Will followup soon with another PR that adds the SVG changes that we need in Expensify App

Kicu commented 3 weeks ago

I also have the SVG changes nicely cleaned up with like 2 extra commits locally, so I will open the 2nd PR with the SVG code on Monday, and this issue should be solved :)

luacmartins commented 3 weeks ago

Nice! Thank you ❤️

grgia commented 2 weeks ago

Thank you @Kicu !

Kicu commented 2 weeks ago

And here is the SVG changes PR: https://github.com/Expensify/react-native-qrcode-svg/pull/207 - I tested via the (now built in) Example app and svg's still work correctly.

Also here is a super simple fix to Example App web version: https://github.com/Expensify/react-native-qrcode-svg/pull/209 please merge this one as well 🙏 - it requires no testing

luacmartins commented 2 weeks ago

Merged both PRs

Kicu commented 2 weeks ago

amazing thanks!

Now we have 1 or 2 things left to do depending on what we want:

How do you want me to proceed? in the long the best thing would to have a dependency to a published version of the package, but someone would have to prepare a release

luacmartins commented 2 weeks ago

I think we should go with a new release and then bump App. I'll check on how we can create a new release for this repo

luacmartins commented 2 weeks ago

I created this PR to add a workflow to automatically publish the lib to npm - https://github.com/Expensify/react-native-qrcode-svg/pull/210

Kicu commented 2 weeks ago

@luacmartins cool, nice job.

Just wanted to make sure that you intended to push to git a file called .github/OSBotify-private-key.asc.gpg? asking since it has private-key in the name 😅

luacmartins commented 2 weeks ago

Yes, that key is required for the workflow to work. It's encrypted though and we have the same in our other repos.

melvin-bot[bot] commented 1 week ago

Triggered auto assignment to @slafortune (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details. Please add this Feature request to a GH project, as outlined in the SO.

melvin-bot[bot] commented 1 week ago

:warning: It looks like this issue is labelled as a New Feature but not tied to any GitHub Project. Keep in mind that all new features should be tied to GitHub Projects in order to properly track external CAP software time :warning:

melvin-bot[bot] commented 1 week ago

Triggered auto assignment to Design team member for new feature review - @dubielzyk-expensify (NewFeature)

bfitzexpensify commented 1 week ago

I am heading out of office until September 21st, so assigning a buddy to watch over this in my absence.

Current status: being worked on.

Kicu commented 4 days ago

Update: this main part of the task:

Migrate changes in react-native-qrcode-svg-old to react-native-qrcode-svg

is basically done, as all the code required to have on main in react-native-qrcode-svg. The only part we're missing is to have nice and clean dependencies is to release a new version of react-native-qrcode-svg and then use this new version in app.

luacmartins commented 3 days ago

We ran into some issues with the automated workflow and we need to fix that first.