Skip to content
This repository has been archived by the owner on Mar 23, 2023. It is now read-only.

feat: multipayment display modals details #1810

Merged
merged 9 commits into from
Mar 14, 2020
Merged

feat: multipayment display modals details #1810

merged 9 commits into from
Mar 14, 2020

Conversation

dated
Copy link
Contributor

@dated dated commented Mar 10, 2020

Summary

TransactionShow

  • aligns the smartbridge to the right and the label to top

TransactionConfirmMultiPayment

  • adds the recipient count
  • adds the total amount
  • adds the vendorfield and fee

TransactionModal

  • limits the max height to 80vh

image
image
image

Checklist

  • Documentation (if necessary)
  • Tests (if necessary)
  • Ready to be merged

@ghost
Copy link

ghost commented Mar 10, 2020

Thanks for submitting this pull request! A maintainer will review this in the next few days and explicitly select labels so you know what's going on.

If no reviewer appears after a week, a reminder will be sent out.

@ghost ghost added Complexity: Low Less than 64 lines changed. Type: Feature The issue is a request for new functionality. labels Mar 10, 2020
brenopolanski
brenopolanski previously approved these changes Mar 10, 2020
View reviewed changes
@ghost ghost added the Status: Member Approved The pull request has been approved by a member. label Mar 10, 2020
@ghost
Copy link

ghost commented Mar 10, 2020

A member has approved this PR. A maintainer will merge this PR shortly. If it shouldn't be merged yet, please leave a comment saying so and we'll wait.

Thank you for your contribution!

dav1app
dav1app previously approved these changes Mar 10, 2020
View reviewed changes
@ghost ghost added the Status: Contributor Approved The pull request has been approved by a contributor. label Mar 10, 2020
@ghost
Copy link

ghost commented Mar 10, 2020

A contributor has approved this PR. A maintainer will merge this PR shortly. If it shouldn't be merged yet, please leave a comment saying so and we'll wait.

Thank you for your contribution!

@dav1app
Copy link
Contributor

dav1app commented Mar 10, 2020

Everything seems ok to me, but I have a scroll bar on the recipients under Debian 10 at 1366x768.

image

On the third screen that you showed above, there is a very small part of the third transaction displayed. I think this can lead the user to ignore the third transaction if the scrollbar isn't displayed. I think that it would be better if you either aways display it or remove the clip at all.

@dated dated dismissed stale reviews from dav1app and brenopolanski via 1ef6452 March 10, 2020 15:57
dav1app
dav1app previously approved these changes Mar 10, 2020
View reviewed changes
@ghost
Copy link

ghost commented Mar 10, 2020

A contributor has approved this PR. A maintainer will merge this PR shortly. If it shouldn't be merged yet, please leave a comment saying so and we'll wait.

Thank you for your contribution!

@alexbarnsley alexbarnsley changed the title feat: multipayment confirmation modal details feat: multipayment modal details Mar 12, 2020
@alexbarnsley alexbarnsley changed the title feat: multipayment modal details feat: multipayment display modals details Mar 12, 2020
@alexbarnsley
Copy link
Member

All looks good, but we think the right-aligned vendorfield text looks a bit odd (I can see why you made it right-aligned, to match the other fields). I think one of the possible alternatives might be better:

  • Revert to left-aligned
  • Justify
  • Move to the bottom (above payment list) and show it how it's shown on the confirmation screen:

image

@alexbarnsley alexbarnsley added the Bounty: Tier 3 Awarded for medium features, refactorings, improvements. This is valued at 50 USD. label Mar 12, 2020
@dated
Copy link
Contributor Author

dated commented Mar 12, 2020

I'll see how it looks with justified alignment. Alignment to the left is a big no-go here imho, it breaks the design too much. Showing it underneath would be the better alternative, compared to left-alignment that is.

@dated
Copy link
Contributor Author

dated commented Mar 12, 2020 •
edited
Loading

I went with text-justify for now @alexbarnsley.

@ghost
Copy link

ghost commented Mar 12, 2020

A contributor has approved this PR. A maintainer will merge this PR shortly. If it shouldn't be merged yet, please leave a comment saying so and we'll wait.

Thank you for your contribution!

@faustbrian faustbrian merged commit 7d8582e into ArkEcosystem:develop Mar 14, 2020
@ghost
Copy link

ghost commented Mar 14, 2020

Your pull request has been merged and marked as tier 3. It will earn you $50 USD.

@dated dated deleted the refactor/multipayment-confirm branch March 14, 2020 09:57
@ghost ghost mentioned this pull request Mar 15, 2020
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@dav1app dav1app dav1app approved these changes

@brenopolanski brenopolanski brenopolanski left review comments

@alexbarnsley alexbarnsley Awaiting requested review from alexbarnsley

@faustbrian faustbrian Awaiting requested review from faustbrian

@ItsANameToo ItsANameToo Awaiting requested review from ItsANameToo ItsANameToo is a code owner

@luciorubeens luciorubeens Awaiting requested review from luciorubeens

Assignees
No one assigned
Labels
Bounty: Tier 3 Awarded for medium features, refactorings, improvements. This is valued at 50 USD. Complexity: Low Less than 64 lines changed. Status: Contributor Approved The pull request has been approved by a contributor. Status: Member Approved The pull request has been approved by a member. Type: Feature The issue is a request for new functionality.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dated @dav1app @alexbarnsley @brenopolanski @faustbrian