-
Notifications
You must be signed in to change notification settings - Fork 23
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
Share to twitter only #899
Conversation
I tested this on my Pixel and this works like a charm! When I hit the Some notes: I used the fix in #901 to deploy to my phone with |
I am going to give this a last shot at testing on my emulator, otherwise I suppose this is ready for review and then merge. I would really like to be able to test it locally to certify that it works |
I've been digging into this and will report my findings shortly - to expand on what @da-kami said earlier, this is the error when trying to fund using 10101 wallet: |
This works on my end, too! 🎉 If twitter is not installed, it redirects to the twitter.com page open in the browser. This is good to know, too. |
Welcome to the club 😥 I'm getting the same error locally and I can't get it working anymore. |
Awesome, if it works. Let's get it in. |
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
are you rebased on top of latest changes? maybe it's time to implement rapid gossip sync 🤔 |
When running on desktop, I get this when trying to share with Twitter:
Maybe this is allowable as we are only targeting mobile? A fallback might not be too hard to implement, though... |
4a3b2cd
to
9416797
Compare
Added a fallback to just use the old solution if platform isn't IOS or Android (SocialShare only works on those two). |
507415a
to
85fc5f6
Compare
@Restioson may I ask you to add a changelog entry for that change? thank you. |
Not sure if this is the best approach - maybe copy to clipboard? Either way probably doesn't matter too much. It's just a bit nicer than throwing errors just in case.
85fc5f6
to
ebe4b47
Compare
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.
Looks good. Let's go
I did it for you :) bors r+ |
899: Share to twitter only r=bonomat a=Restioson Use the `social_share` package to share directly and specifically to twitter instead of opening a general share dialogue. This PR is currently a draft since I haven't been able to test this on the Android emulator due to unrelated problems with its setup. Fixes #824. Co-authored-by: Restioson <[email protected]>
Build succeeded! The publicly hosted instance of bors-ng is deprecated and will go away soon. If you want to self-host your own instance, instructions are here. If you want to switch to GitHub's built-in merge queue, visit their help page. |
Thanks! |
Closing as this has been merged |
Use the
social_share
package to share directly and specifically to twitter instead of opening a general share dialogue. This PR is currently a draft since I haven't been able to test this on the Android emulator due to unrelated problems with its setup.Fixes #824.