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: boost button in websites #1939

Open
wants to merge 27 commits into
base: master
Choose a base branch
from
Open

feat: boost button in websites #1939

wants to merge 27 commits into from

Conversation

im-adithya
Copy link
Member

@im-adithya im-adithya commented Jan 4, 2023

Describe the changes you have made in this PR

This PR replaces the Blue Extension Icon on finding a Lightning Address to have a Boost Sats button in the bottom right corner.

(Can close #1516 if we merge this)

Linked Issue

Type of change

(Remove other not matching type)

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

Screenshots of the changes [optional]

Screen.Recording.2023-03-16.at.12.16.05.AM.mov

How has this been tested?

Tested manually in each site:

Don't have links to check:

  • GeyserProject:
  • Mixcloud:
  • PeerTube:
  • Vimeo:
  • Monetization:

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)

@im-adithya im-adithya marked this pull request as draft January 4, 2023 16:37
@github-actions
Copy link

github-actions bot commented Jan 4, 2023

🚀 Thanks for the pull request!

Here are the current build files for testing:

Download and unzip the file for your browser. Refer to the readme for detailed install instructions.


This build is brought to you by: channel.ninja (who recently dropped 1000 sats):


Want to sponsor the next build? send some sats to ⚡️builds@getalby.com (don't forget to provide your name)

Don't forget: keep earning sats!

@rolznz
Copy link
Contributor

rolznz commented Jan 5, 2023

@im-adithya I tried this out locally

  • I think there needs to be a close button, especially if it blocks other content on the page like other floating buttons
  • Should the button show if you have a new install of the extension but haven't created an account yet? It does redirect to the account creation page though so it might be fine as it is.
  • (Future idea) I think it would be cool to see how many tips you have sent them so far, and also how many boosts/total amount of boosts they have received in total (I guess the publisher would need to expose this publicly somehow)

@kiwiidb
Copy link
Contributor

kiwiidb commented Jan 6, 2023

I've also tested out the boost button, nice work!

  • If the Alby window opens I don't see the profile picture of the user but the icon of the website (except on Vida?!). Would be nice to have the profile picture in there.
  • I find the layout to be a bit too verbose and also not extremely noticeable. Sometimes it takes a bit to load and if you're then already reading on the website you might miss it. Maybe an initial animation would be cool?

@pavanjoshi914
Copy link
Contributor

pavanjoshi914 commented Mar 12, 2023

@im-adithya can you check if this is working for youtube videos for me This isn't working for the same. did check out youtube videos battery where we set lightning data using setLightningData, but when we try to extract lightning data from the boost button component using extractLightningData it returns ```undefined and leads to the invisibility of the boost button

@im-adithya im-adithya marked this pull request as ready for review March 15, 2023 18:48
@reneaaron
Copy link
Contributor

Nice work @im-adithya, I just tested the PR and feels great already. 🚀

Some first feedback on the UI:

  • I think we should remove the infinite loop animation of the lightning bolt but instead just play it once (after the button is initialized). It constantly draws attention to this button and distracts (especially if you are reading content or watching a video)
  • The shadows are too big and dark, also some spacing around wouldn't hurt (see @stackingsaunter's designs)
  • The font doesn't seem to be loaded and the text in the tooltip is very small: (firefox)
    image
  • I think we need a loading state between the last click and displaying the payment prompt (maybe we can show a spinning wheel here?)

Can't wait to see zaps all over the web. 🙌 I'll follow up with a code review later this week.

@rolznz
Copy link
Contributor

rolznz commented Mar 16, 2023

@im-adithya
I see there are two different methods to pay (sendSatsToLnurl - direct webLN payment to lightning address and generateInvoice - using Alby tools).

Is it necessary to have both? I think this will be harder to maintain and we'd need two different ways to attach metadata. The alby-tools version is better in my opinion as this seems easier to add LUD-18 data by passing payerData to generateInvoice. What do you think?

@rolznz
Copy link
Contributor

rolznz commented Mar 16, 2023

@im-adithya it would be great to convert the boost button to typescript in a follow up PR 🚀

@rolznz
Copy link
Contributor

rolznz commented Mar 16, 2023

@im-adithya if you send a payment, is it impossible to send another without refreshing the page?

image

@pavanjoshi914
Copy link
Contributor

looks great just tested out!!
few of the things to be noticed

  1. no WebLN error after payment window is closed
  2. personally i felt i had to press too long for custom payment time can be reduced
    screen-capture.webm


// extract LN data from websites
browser.runtime.onMessage.addListener((request, sender, sendResponse) => {
if (request.action === "extractLightningData") {
extractLightningData();
// extractLightningData();
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this intentional or a mistake? If intentional, can't you delete the entire if and the event listener then?

@stackingsaunter
Copy link
Contributor

stackingsaunter commented Mar 16, 2023

Great work! I love the tingling animation :D

I have a few thoughts:

  1. I second @reneaaron comments regarding shadow and hover font

  2. After I cancel payment, there's this and it doesn't work until I refresh. Does it have to work that way?
    Screenshot 2023-03-16 at 22 03 26

  3. After the popup appears with payments, I still can click and increase zap, which does nothing, this feels buggy a bit. I think the increases should be blocked then
    Screenshot 2023-03-16 at 22 05 33

  4. After I sent zap, it's stuck in this state and I can't do it again. I think it'd be better if the zap resteted to default state. Is it possible? However, the confirmation on the button (x sats sent) should be visible (unwinded) without hovering and then going back to defaults state. The reason for this is that people with budget set will have no visual clue they paid anything (if they don't hover)
    Screenshot 2023-03-16 at 22 13 14

  5. I second @pavanjoshi914 that hodl should be MUCH shorter. I think basically any holding which is not a single click on a button should trigger custom amount popup

  6. On twitter it interferes with Messages. I think the button shouldn't go public without Twitter and YT integration.
    Screenshot 2023-03-16 at 22 24 34

  7. I think we need to add customisation in settings regarding the default boost increase with each click. Some users will prefer smaller or higher amounts. I propose something like this:

Screenshot 2023-03-16 at 22 16 23

@socket-security
Copy link

socket-security bot commented May 10, 2023

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

@im-adithya
Copy link
Member Author

All declarations of 'webln' must have identical modifiers.ts(2687)
Subsequent property declarations must have the same type. Property 'webln' must be of type 'WebLNProvider | undefined', but here has type 'WebLNProvider'.ts(2717)
index.d.ts(86, 5): 'webln' was also declared here.

We have removed the webln declaration in src/extension/providers/webln/index.ts as alby-tools has a dependency (webbtc) that types this beforehand. (cc @roland). Adding this here if anything breaks because of removing alby-tools in case in future.

@stackingsaunter stackingsaunter self-assigned this Aug 21, 2023
@stackingsaunter stackingsaunter removed their request for review August 21, 2023 19:04
@socket-security
Copy link

socket-security bot commented Aug 22, 2023

No top level dependency changes detected. Learn more about Socket for GitHub ↗︎

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