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: lnurlpay screen toggle more fields #2373

Merged
merged 2 commits into from
Apr 26, 2023

Conversation

lujakob
Copy link
Contributor

@lujakob lujakob commented Apr 25, 2023

Describe the changes you have made in this PR

Toggle more fields on lnurlpay screen to have buttons visible.

Link this PR to an issue

Fixes #2370

Type of change

(Remove other not matching type)

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

Screenshots of the changes [optional]

Bildschirmfoto 2023-04-24 um 21 07 04

Bildschirmfoto 2023-04-24 um 21 07 12

How has this been tested?

Manually

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 Apr 25, 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!

@lujakob
Copy link
Contributor Author

lujakob commented Apr 25, 2023

@stackingsaunter regarding your requirements:

  • Must involve all transacting screens with optional fields hidden in this way in the popup view
    I checked all screens that are linked from the "Send" screen and found none, except lnurlpay screen (which is the one in your design). I have nothing in mind and am not aware of how to skip through all possibilities/screens. Is there anything I missed?

  • Link default state color is blue600 and blue700 on hover (both light and dark theme)
    As the recommended colors didn't show up in the IDE classes auto suggestion I searched for font color classes in the project and found the Hyperlink component utilizing text-blue-700 hover:text-blue-500. The only place this component is used is in Unlock and ConfirmSignMessage. Anyways, I just used the Hyperlink here as well.

@stackingsaunter
Copy link
Contributor

  • Must involve all transacting screens with optional fields hidden in this way in the popup view

I am not aware how it is structured in the code. I know that different optional fields appear depending on LNURL, eg:
Screenshot 2023-04-25 at 12 12 35
But it seems it works for all in this build!

  • I just used the Hyperlink here as well.

That's OK, Rene will update hyperlink colors separately

Thanks @lujakob for this PR!

@stackingsaunter
Copy link
Contributor

@lujakob hey I discussed the change with @reneaaron and we decided to pull of "Comment" input out of "More", as this is quite often used and important field. Could you move it out?

Screenshot 2023-04-25 at 14 59 42

This will make users still need to scroll, but I am working on changing other elements (ln address box) to make it all fit, but that will be a seperate issue.

Also, could you delete the 1px border/divider above the buttons as well? Thank you!

Screenshot 2023-04-25 at 15 02 40

@lujakob
Copy link
Contributor Author

lujakob commented Apr 25, 2023

@lujakob hey I discussed the change with @reneaaron and we decided to pull of "Comment" input out of "More", as this is quite often used and important field. Could you move it out?

This will make users still need to scroll, but I am working on changing other elements (ln address box) to make it all fit, but that will be a seperate issue.

Also, could you delete the 1px border/divider above the buttons as well? Thank you!

@stackingsaunter

  • I moved the comment field to always display
  • I removed the border as suggested
  • I cleaned up the margin/paddings. At least it's possible to notice there is more UI coming (see screens)

Bildschirmfoto 2023-04-25 um 12 17 56

Bildschirmfoto 2023-04-25 um 12 18 13

Copy link
Contributor

@stackingsaunter stackingsaunter left a comment

Choose a reason for hiding this comment

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

tACK

@reneaaron
Copy link
Contributor

Thanks @lujakob, awesome job! 👍 (Great that you extracted those inline checks into methods, a lot easier to read now!)

tACK

@reneaaron reneaaron merged commit 5eba6ae into getAlby:master Apr 26, 2023
bumi added a commit that referenced this pull request Apr 29, 2023
* master:
  fix: change link color to blue-600 / blue-700 on hover (#2372)
  feat: lnurlpay screen toggle more fields (#2373)
  chore: yarn lock
  fix: wait for transactions to load
  fix: use account id from connector directly
  fix: tabs hover / dark mode (#2360)
  feat: add account-context balanceLoading (#2359)
  yarn lock file
  bug: show no txs msg after loading
  bug: show no txs  msg after loading
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.

Hide optional inputs under "More" button on transacting screens
3 participants