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

Show BSQ trading fee more explicitly when funding your offer #2379

Merged
merged 4 commits into from Feb 11, 2019

Conversation

Projects
None yet
2 participants
@ripcurlx
Copy link
Member

ripcurlx commented Feb 6, 2019

  • Fixes #2075.
  • Fixes wrong total funds value in info popup and funds tooltip
  • Fixes missing info icon in Funds needed field
  • Fixes positioning of info icons

ripcurlx added some commits Feb 5, 2019

@ripcurlx ripcurlx requested a review from ManfredKarrer as a code owner Feb 6, 2019

@ripcurlx ripcurlx added the in:dao label Feb 6, 2019

@ripcurlx ripcurlx force-pushed the ripcurlx:improve-fund-your-offer-popup branch from 700a854 to f43032d Feb 6, 2019

@ManfredKarrer ManfredKarrer requested a review from sqrrm Feb 6, 2019

@sqrrm

sqrrm approved these changes Feb 10, 2019

Copy link
Member

sqrrm left a comment

utACK

@@ -373,6 +373,7 @@ createOffer.fundsBox.networkFee=Mining fee
createOffer.fundsBox.placeOfferSpinnerInfo=Offer publishing is in progress ...
createOffer.fundsBox.paymentLabel=Bisq trade with ID {0}
createOffer.fundsBox.fundsStructure=({0} security deposit, {1} trade fee, {2} mining fee)
createOffer.fundsBox.fundsStructure.BSQ=({0} security deposit, {1} mining fee) + {2} trade fee

This comment has been minimized.

@sqrrm

sqrrm Feb 10, 2019

Member

Maybe this should keep the same order (deposit, trade fee, mining fee) as createOffer.fundsBox.fundsStructure for less confusion?

This comment has been minimized.

@ripcurlx

ripcurlx Feb 11, 2019

Author Member

The reason why I did it like that is, that it would otherwise look like as if it is part of the total btc funds needed.
bildschirmfoto 2019-02-11 um 09 54 54
So I implemented it as follows:
bildschirmfoto 2019-02-11 um 09 56 30

This comment has been minimized.

@sqrrm

sqrrm Feb 11, 2019

Member

Ah, perhaps the ordering should be deposit, mining, trade fee for both?

This comment has been minimized.

@ripcurlx

ripcurlx Feb 11, 2019

Author Member

On first sight yes, but we have this order on a couple of places and it makes sense from my point of view to have it as it is right now:
bildschirmfoto 2019-02-11 um 15 12 05

bildschirmfoto 2019-02-11 um 15 11 08

bildschirmfoto 2019-02-11 um 15 14 44

The only problem is to mix it in the total funds field with the BSQ value.
I personally would go with this little inconsistency and have the order in this field different between BSQ and BTC trading fees.

This comment has been minimized.

@sqrrm

sqrrm Feb 11, 2019

Member

Yeah, that's a lot to change, then just go with your original suggestion unless we have an obviously better way to do it.

@ripcurlx ripcurlx merged commit f7f4f07 into bisq-network:master Feb 11, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ripcurlx ripcurlx referenced this pull request Feb 26, 2019

Closed

For February 2019 #224

@sqrrm sqrrm referenced this pull request Feb 28, 2019

Closed

For February 2019 #228

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.