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: improve LNURL ux #2152

Merged
merged 3 commits into from
Mar 6, 2023
Merged

feat: improve LNURL ux #2152

merged 3 commits into from
Mar 6, 2023

Conversation

reneaaron
Copy link
Contributor

Describe the changes you have made in this PR

Have the LNURL payment form respect the provided min and max values.

  • Add the values to the form input
  • Disable SatsButtons that are out of the range

Link this PR to an issue [optional]

Fixes #1569

Type of change

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

Screenshots of the changes [optional]

image

How has this been tested?

LNURLs used for testing

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)

@github-actions
Copy link

github-actions bot commented Feb 28, 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!

@reneaaron reneaaron marked this pull request as ready for review February 28, 2023 16:54
@reneaaron reneaaron requested a review from rolznz February 28, 2023 22:54
@@ -420,6 +420,8 @@ function LNURLPay() {
fiatValue={fiatValue}
/>
<SatButtons
min={Math.floor(+details.minSendable / 1000)}
Copy link
Contributor

Choose a reason for hiding this comment

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

is minSendable always available? undefined / 1000 = NaN

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think min / max are required fields for LNURL responses. Other components on this form use those value the same way:

https://github.com/getAlby/lightning-browser-extension/pull/2152/files#diff-0ae7727e94b1752f52e903368c32a80af0ae7d22e7d4d4e17bb6b7219fe7a076R415-R416

@escapedcat
Copy link
Contributor

Does this make #1697 obsolete?

@reneaaron
Copy link
Contributor Author

Does this make #1697 obsolete?

I think so, yes. It just makes more sense to me (and is a lot less complicated I guess).

<Button
key={size}
icon={<SatoshiV2Icon className="w-4 h-4" />}
label={size / 1000 + "k"}
Copy link
Collaborator

Choose a reason for hiding this comment

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

so the head of Emoji is removing the ⚡️ emojis here?

@bumi bumi merged commit 6d3dd36 into master Mar 6, 2023
@bumi bumi deleted the feat/lnurl-min-max branch March 6, 2023 12:57
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.

Use minSendable and maxSendable values from LNURLp response to cap user input
4 participants