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

Added Alby Go to mobile wallets selection #2668

Closed
wants to merge 11 commits into from

Conversation

itstomekk
Copy link
Contributor

Summary

Added Alby Go to mobile wallets selection

Checklist

  • [x ] I have read (or I am familiar with) the Contribution Guidelines
  • I have tested the changes in this PR - not sure about albygo::lightning handle, reviewing with alby team
  • [x ] My PR is either small, or I have split it into smaller logical commits that are easier to review
  • I have added the signoff line to all my commits. See Signing off your work
  • I have added appropriate changelog entries for the changes in this PR. See Adding changelog entries
    • [x ] I do not need to add a changelog entry. Reason: [very small change]
  • [x ] I have added appropriate Closes: or Fixes: tags in the commit messages wherever applicable, or made sure those are not needed. See Submitting patches

Other notes

Simple patch, my first here, I'm not very experienced with github

@jb55
Copy link
Collaborator

jb55 commented Nov 8, 2024

looks good to me!

@jb55
Copy link
Collaborator

jb55 commented Nov 15, 2024

should be good to merge @danieldaquino ?

@danieldaquino
Copy link
Contributor

Thank you @itstomekk for the PR!

I gave it a try, but I noticed the logo is not showing up for some reason:

Screenshot 2024-11-15 at 12 42 48

I also noticed this error message:

No image named 'albygo' found in asset catalog for /Users/*****/Library/Developer/CoreSimulator/Devices/***********/data/Containers/Bundle/Application/************/damus.app

I believe there is a typo on the image name. When I apply this change locally the image appears:

diff --git a/damus/Models/Wallet.swift b/damus/Models/Wallet.swift
index 7693cb1c..5d82373c 100644
--- a/damus/Models/Wallet.swift
+++ b/damus/Models/Wallet.swift
@@ -93,7 +93,7 @@ enum Wallet: String, CaseIterable, Identifiable, StringCodable {
                          appStoreLink: "https://apps.apple.com/us/app/river-buy-mine-bitcoin/id1536176542", image: "river")
         case .albygo:
             return .init(index: 13, tag: "albygo", displayName: "Alby Go", link: "albygo:lightning:",
-                         appStoreLink: "https://apps.apple.com/us/app/alby-go/id6471335774", image: "albygo")
+                         appStoreLink: "https://apps.apple.com/us/app/alby-go/id6471335774", image: "alby-go")
             
         }
     }

Furthermore, I tried installing Alby Go on my phone and going through the link, but it always takes me to the App store page. Perhaps it would be best for us to wait for the Alby team's response on the albygo:lightning: handle before we merge this.

Can you please check these?

Thank you!

@danieldaquino danieldaquino self-requested a review November 15, 2024 22:20
Copy link
Contributor

@danieldaquino danieldaquino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Marking this PR as "requested changes" to help me keep track of which PRs I need to look at. My review comments are in a previous comment)

@jb55
Copy link
Collaborator

jb55 commented Nov 16, 2024 via email

@itstomekk itstomekk marked this pull request as ready for review January 3, 2025 14:28
@itstomekk itstomekk marked this pull request as draft January 3, 2025 14:28
Copy link
Contributor Author

@itstomekk itstomekk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@itstomekk itstomekk marked this pull request as ready for review January 5, 2025 14:06
@danieldaquino
Copy link
Contributor

Thank you @itstomekk! I confirm that the icon issue has been fixed!

However, I still had issues with opening the Alby Go link with Alby Go installed on my phone. I did a bit of digging and I found that this patch on to top of your PR seems to fix that:

diff --git a/damus/Info.plist b/damus/Info.plist
index e77d88a3..fb84175d 100644
--- a/damus/Info.plist
+++ b/damus/Info.plist
@@ -48,6 +48,7 @@
 	<key>LSApplicationQueriesSchemes</key>
 	<array>
 		<string>river</string>
+		<string>alby</string>
 		<string>albygo</string>
 		<string>bitcoinbeach</string>
 		<string>breez</string>
diff --git a/damus/Models/Wallet.swift b/damus/Models/Wallet.swift
index 83b33ff6..1c2552c4 100644
--- a/damus/Models/Wallet.swift
+++ b/damus/Models/Wallet.swift
@@ -92,7 +92,7 @@ enum Wallet: String, CaseIterable, Identifiable, StringCodable {
             return .init(index: 12, tag: "river", displayName: "River", link: "river://",
                          appStoreLink: "https://apps.apple.com/us/app/river-buy-mine-bitcoin/id1536176542", image: "river")
         case .albygo:
-            return .init(index: 13, tag: "albygo", displayName: "Alby Go", link: "alby://",
+            return .init(index: 13, tag: "albygo", displayName: "Alby Go", link: "alby:",
                          appStoreLink: "https://apps.apple.com/us/app/alby-go/id6471335774", image: "alby-go")
             
         }

Other than that, the code itself is good for merging. The commits need a bit of tweaking to fully adhere to our contribution guidelines (https://github.com/damus-io/damus/blob/master/docs/CONTRIBUTING.md), but I can easily take care of that for you this time since it's your first PR with us, so don't worry!

I just need one last thing: Our guidelines require contributors to signoff their contributions (See https://github.com/damus-io/damus/blob/master/docs/CONTRIBUTING.md#sign-your-work---the-developers-certificate-of-origin)

Usually developers should do that while committing via git commit --signoff, but this time you can just send me that via a comment, and I can copy/paste it to the commits. Can you please send me a line like in the example below, with your name and email on it?

Signed-off-by: Random J Developer <[email protected]>

Thank you!

@itstomekk
Copy link
Contributor Author

Thank you @danieldaquino !

Signed-off-by: Tomek K <[email protected]>

@danieldaquino
Copy link
Contributor

Performed the final needed tweaks and merged your changes manually! It's now merged on master in e106be1

Thank you @itstomekk!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants