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

feat(tipping): github profile support #767

Merged
merged 2 commits into from
Apr 11, 2022

Conversation

reneaaron
Copy link
Contributor

@reneaaron reneaaron commented Apr 1, 2022

Link this PR to an issue

Fixes #766

Type of change (Remove other not matching type)

  • New feature (non-breaking change which adds functionality)

Describe the changes you have made in this PR -

Currently we only support Github profiles. We could widen support to repositories (so you have the option to donate on i.e. reneaaron/webln-demo at the cost of an extra request, too.

How Has This Been Tested?

  • Add a lightning address to your GitHub profile

Checklist:

  • My code follows the style guidelines of this project and performed a self-review of my own code
  • New and existing tests pass locally with my changes
  • I checked if I need to make corresponding changes to the documentation (and made those changes if needed)

@reneaaron reneaaron marked this pull request as ready for review April 1, 2022 13:02
@reneaaron
Copy link
Contributor Author

@bumi Support for GitHub profiles is here! 💯

Do you think we should support donation on user repositories for the price of an extra request? Since currently we try to avoid that I initially skipped that, but I am happy to add it.

Regarding docs update:
I've updated the table here, do we need to add info about this battery to other places as well?

@escapedcat escapedcat requested a review from bumi April 2, 2022 07:00
Copy link
Collaborator

@bumi bumi left a comment

Choose a reason for hiding this comment

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

as the URL matches for both repos and profiles. Maybe we can directly also read the data from the readme?

so this one would work for both, user profiles and repos.

@bumi
Copy link
Collaborator

bumi commented Apr 2, 2022

I would also delete the existing one: https://github.com/getAlby/lightning-browser-extension/blob/master/src/extension/content-script/batteries/GitHubRepo.ts - it used the lndonate idea of a file. but requires an additional HTTP request and imo it is good to put the info in the readme.

for a second I also thought we could additionally load the info from the .github/FUNDING file (check for a link that has a lighting=... parameter.
(see https://github.com/bumi/lnme/blob/master/.github/FUNDING.yml

@reneaaron
Copy link
Contributor Author

reneaaron commented Apr 2, 2022

for a second I also thought we could additionally load the info from the .github/FUNDING file (check for a link that has a lighting=... parameter.

I didn't know about that. That is awesome! 💯 For supporting the FUNDING.yml I'd probably create another issue as I think we need to put some more thought into that. (how to identify lightning addresses, lnurlp, etc)

as the URL matches for both repos and profiles. Maybe we can directly also read the data from the readme?

That should be possible, too.

So we would have the following options for now:

  • GitHub Profile via⚡ in short bio or README in username/username repository
  • GitHub Repos via ⚡in README (on repository startpage)

@bumi
Copy link
Collaborator

bumi commented Apr 2, 2022

For supporting the FUNDING.yml I'd probably create another issue as I think we need to put some more thought into that. (how to identify lightning addresses, lnurlp, etc)

+1 - and it already works when the user clicks on the sponsor link. Alby will open with such lightning query params.

import getOriginData from "../originData";
import setLightningData from "../setLightningData";

const urlMatcher = /^https:\/\/github.com\/([^/]+)(\/([^/]+))?$/;
Copy link
Collaborator

Choose a reason for hiding this comment

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

(I only found that that this regex does not match on github.com/username/ (with a trailing slash) - but this happens only when manually entered. )


const address = parseElement("[data-bio-text]") || parseElement("article");

if (address) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I kinda prefer the guards like in line 31 - if there is no else branch

@bumi bumi merged commit 2de6242 into getAlby:master Apr 11, 2022
Copy link
Collaborator

@bumi bumi left a comment

Choose a reason for hiding this comment

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

@reneaaron I was too fast with merging... :/
do you have time to update the CSS selectors and make those more specific?

@reneaaron
Copy link
Contributor Author

@bumi Sure, i'll have a look at it!

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.

Tipping support for GitHub profiles
3 participants