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

[redesign][ln] Send tab #3538

Merged
merged 16 commits into from Aug 24, 2021
Merged

[redesign][ln] Send tab #3538

merged 16 commits into from Aug 24, 2021

Conversation

bgptr
Copy link
Collaborator

@bgptr bgptr commented Jul 23, 2021

Toward issue #3296

This diff updates only the header and the send form. I did not touch the history lists (Ongoing payments, Failed payments, Latest payments) since the design doesn't contain them.
update: @MariaPleshkova has added the list to the Figma designs, and this diff implements it.

Updated screenshots:
DeepinScreenshot_select-area_20210723100235
DeepinScreenshot_select-area_20210818143908
DeepinScreenshot_select-area_20210818143929
DeepinScreenshot_select-area_20210818143941

@bgptr bgptr marked this pull request as ready for review August 1, 2021 08:51
@bgptr bgptr marked this pull request as draft August 4, 2021 12:41
@bgptr bgptr marked this pull request as ready for review August 16, 2021 10:19
Copy link
Member

@matheusd matheusd left a comment

Choose a reason for hiding this comment

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

I feel you need to show more info about the decoded payment request. At the very least, you need to show the payment hash, description and the expiry.

Showing the extended details (ctlv expiry, fallback onchain address, etc) could be added in an expanded view.

It would probably be helpful to have the destination node use the copyable component as well.

You should also block attempting to pay for expired invoices (while the node will ultimately reject the payment attempt, it's better to also do it in the earliest stage possible, with feedback to the user of what's going on).

- copyable destination 
- add expiry and description fields 
- block expired invoices to pay
@bgptr
Copy link
Collaborator Author

bgptr commented Aug 17, 2021

Thanks, @matheusd. I've added the suggested details and blocked attempting to pay for expired invoices.
Screenshots are updated. @MariaPleshkova, take a look at them if you'll have time, please.

Copy link
Member

@matheusd matheusd left a comment

Choose a reason for hiding this comment

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

Looking good! Only two additional nits.

@bgptr
Copy link
Collaborator Author

bgptr commented Aug 17, 2021

@matheusd, thanks. I've pushed the changes.

@MariaPleshkova
Copy link

@bgptr generally looks good, but a few small tweaks from my side:

  1. Changed "Expiry" title to the "Expiration Time" as we already use it this way.
  2. Added a vertical stroke between "Amount-Destination" area and "Expiration Time".
  3. "Description" and "Payment Hash" areas have to be visually similar to another info areas within this section (expiration time, destination, etc.). I mean that the titles should be located above their description text, not to the left from it.

image

@MariaPleshkova
Copy link

@bgptr and check the spacings within Lightning Payment Request Code, please. They should be 10px from all sides:
image

@MariaPleshkova
Copy link

And one more thing: let's show success and error states in a bit different way.
You used orange and green icons(with an exclamation mark and a tick) on your screenshots, but we don't use them on the layouts in Figma for these fields.
image
image

Instead we change the color of a field stroke to green and add a green description under it for success states, and change the stroke color+an entered text to orange and add an orange description under it as well for error states.
image
image

- fix padding of the CopyableText 
- improve req code textinput (hide icons)
- improve decoded pay request form
@bgptr
Copy link
Collaborator Author

bgptr commented Aug 18, 2021

@MariaPleshkova, thanks for the review. I've implemented your suggestions and updated the screenshots.

Copy link
Member

@matheusd matheusd left a comment

Choose a reason for hiding this comment

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

Final typo, but otherwise LGTM.

@alexlyp alexlyp merged commit 4d25cd5 into decred:master Aug 24, 2021
@bgptr bgptr deleted the ln-redesign-send branch August 27, 2021 16:22
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