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: success screen component (LNURLPay) #1446

Merged
merged 4 commits into from Sep 27, 2022
Merged

Conversation

im-adithya
Copy link
Member

@im-adithya im-adithya commented Sep 14, 2022

Describe the changes you have made in this PR

Adds a new success/failure card component

Link this PR to an issue

#1379

Type of change (Remove other not matching type)

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

Screenshots of the changes (If any)

How has this been tested?

Have to check the Failure screen (only checked it by throwing a hardcoded error)

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 changed the title Task success screen success screen component Sep 14, 2022
@im-adithya im-adithya changed the title success screen component feat: success screen component Sep 14, 2022
@github-actions
Copy link

github-actions bot commented Sep 14, 2022

🚀 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: null (who recently dropped 210 sats):

Checkout the Alby Lightning Mixtape: https://alby-mixtape.vercel.app - happy coding!

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

Don't forget: keep stacking sats!

Copy link
Contributor

@escapedcat escapedcat left a comment

Choose a reason for hiding this comment

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

  • Would be nice to get the icons as SVGs to avoid future scaling issues and make reusability easier
  • Will the ResultCard be applied to all "send" screens in further PRs?

src/app/screens/LNURLPay/index.tsx Outdated Show resolved Hide resolved
src/app/components/ResultCard/index.tsx Outdated Show resolved Hide resolved
@im-adithya im-adithya changed the title feat: success screen component feat: success screen component (LNURLPay) Sep 15, 2022
@im-adithya im-adithya force-pushed the task-success-screen branch 2 times, most recently from 206ef92 to 657313a Compare September 19, 2022 06:34
@escapedcat escapedcat merged commit 73bd1e5 into master Sep 27, 2022
@escapedcat escapedcat deleted the task-success-screen branch September 27, 2022 11:16
@bumi
Copy link
Collaborator

bumi commented Sep 27, 2022

when does this error happen? and from the screenshot it seems the user is in a dead end there? typically a user would want to retry it? also why do we show errors here differently than in the other screens where the error message is shown under the input?

@escapedcat
Copy link
Contributor

when does this error happen?

I think this was just an example-message to show how that screen looks like, right @im-adithya ?

and from the screenshot it seems the user is in a dead end there?
typically a user would want to retry it?

You mean because before a toast was used and the user could try again? If yes, then instead of "close" on the error-screen, let's use "Retry" and the user will see the form with the input again?

also why do we show errors here differently than in the other screens where the error message is shown under the input?

Let me talk to @dvoroneca

@bumi
Copy link
Collaborator

bumi commented Sep 28, 2022

:) that does not answer my question of when this error happens.

I question the need of this whole screen. before it was a toast() and now it is much more code, we need to navigate with some new logic on a rather hidden button, etc.
@dvoroneca is this really better? and is this really so much better that it is worth spending the time here?

@escapedcat
Copy link
Contributor

:) that does not answer my question of when this error happens.

Ah, yeah, I didn't get you meant the whole error-screen. Got it! Yes, valid point. Will talk to Diana/Adithya.

@dvoroneca
Copy link

A valid concern. Would need to find out real cases when the transaction doesn't go through and other errors to answer this question.

@escapedcat
Copy link
Contributor

Maybe errors in general do not need to be blocking. Errors can be shown below i.e. the form.
Currently we use a mix of toast and "show below form".
We can still align the success-screens for all "send"-types.

@bumi
Copy link
Collaborator

bumi commented Sep 28, 2022

A valid concern. Would need to find out real cases when the transaction doesn't go through and other errors to answer this question.

why do the cases when a transaction goes wrong matter?

@dvoroneca
Copy link

dvoroneca commented Sep 29, 2022

The designs of these screens had a CTA specific for cases when an error occurs. File here
Screenshot 2022-09-29 at 13 05 32

There may be cases where the connection is lost, the node is down or other technical problems prevent the funds from being sent. It is true that we already have an error popup that may be displayed on the screen before that (where there is data entry and a call to action for payment), but this window has no CTA to direct the user to, for example, some resource.... help.

It is also a way for us to find out what kind of errors users encounter. When they contact the helpdesk and say that there was an error when they sent the funds.

Not being able to make payments is a bad UX and users may want to try another service to send funds if Alby isn't working. Please let me know if I have missed anything.

@dvoroneca
Copy link

How about we roll back the error screen for now and leave the success screen? @escapedcat @escapedcat @bumi

@escapedcat
Copy link
Contributor

How about we roll back the error screen for now and leave the success screen?

Yeah, that would be great. At least in this case the design isn't wasted and we finally can align the send-screens in a way.

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.

None yet

4 participants