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: add receive via bitcoin address button #2494

Merged
merged 5 commits into from
Jun 26, 2023

Conversation

pavanjoshi914
Copy link
Contributor

Describe the changes you have made in this PR

add receive via bitcoin address button
styling button
adding translations
add ReceiveOnChain component
resolve missing key prop for translations
show button only when its an alby account

Type of change

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

Screenshots of the changes [optional]

image
image

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
  • 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 Jun 16, 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.

Don't forget: keep earning sats!

@rolznz
Copy link
Contributor

rolznz commented Jun 19, 2023

@stackingsaunter do you have a figma file @pavanjoshi914 can review to ensure he gets the styles right?

image

For example, I think the action button normally should be at the bottom of the popup (to be confirmed)

@stackingsaunter
Copy link
Contributor

Let's put the button at the bottom and maybe center the text on the screen (so it's in the middle not on the top). Not super elegant but I think this is temporary solution anyway, as soon we can have swaps within the extension, so let's just merge it!

@pavanjoshi914
Copy link
Contributor Author

pavanjoshi914 commented Jun 21, 2023

Let's put the button at the bottom and maybe center the text on the screen (so it's in the middle not on the top). Not super elegant but I think this is temporary solution anyway, as soon we can have swaps within the extension, so let's just merge it!

do this looks good? cc @rolznz @stackingsaunter
image

@rolznz
Copy link
Contributor

rolznz commented Jun 21, 2023

@pavanjoshi914 I found one minor spacing issue. I think the OR + buttons should be aligned at the bottom. @stackingsaunter could you confirm?

image

Otherwise looks good to me!

@rolznz rolznz requested a review from reneaaron June 21, 2023 08:25
Copy link
Contributor

@reneaaron reneaaron left a comment

Choose a reason for hiding this comment

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

Let's do another feedback loop and review the copy of this PR, other than that:

tACK

@stackingsaunter
Copy link
Contributor

Regarding layout, not sure which feels better. @reneaaron @pavanjoshi914 @rolznz thoughts?

image

Besides that small suggested small changes, otherwise tACK

  • Change "or" to small caps
  • Change screen title to "Receive via bitcoin address"
  • Change first part of copy to "To receive Bitcoin via standard address, log-in to Alby Account on getalby.com"

image

@pavanjoshi914
Copy link
Contributor Author

thanks for the changes. shall we replacd the "or" in lnd screen as "or" is placed in common set of strings now

image

pavanjoshi914 and others added 4 commits June 23, 2023 12:35
styling button
adding translations
add ReceiveOnChain component
resolve missing key prop for translations
signed-off-by: Pavan Joshi <pavanj914@gmail.com>
handle conditional rendering of button
center align text and button at button in onchainReceive Screen
resolve problem of loading state while creating invoice
signed-off-by: Pavan Joshi <pavanj914@gmail.com>
signed-off-by: Pavan Joshi <pavanjoshi914@gmail.com>
@rolznz
Copy link
Contributor

rolznz commented Jun 23, 2023

@pavanjoshi914 I think it is fine to change to "or". It might be nice to have a Divider component, and then we could also use it in the LND connect screen. What do you guys think? (Maybe this could be done as a follow up PR?)

@pavanjoshi914
Copy link
Contributor Author

@pavanjoshi914 I think it is fine to change to "or". It might be nice to have a Divider component, and then we could also use it in the LND connect screen. What do you guys think? (Maybe this could be done as a follow up PR?)

let's do this in follow up pr in think

@pavanjoshi914
Copy link
Contributor Author

@reneaaron can we merge this?

@reneaaron reneaaron merged commit 06a73b2 into getAlby:master Jun 26, 2023
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.

4 participants