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

Add "Buy BSQ" button next to trade fee selector #5055

Merged
merged 25 commits into from Nov 8, 2021

Conversation

Jakub-CZ
Copy link
Contributor

@Jakub-CZ Jakub-CZ commented Jan 4, 2021

Fixes #4579

New button is added in the CREATE OFFER and TAKE OFFER tabs. When clicked, it shows a popup and then brings the user to the Sell Offer Book (SELL BITCOIN), with BSQ preselected in the currency filter.

The button is visible only when the fee currency toggles are visible (relevant in the CREATE OFFER tab).

Screenshots

start creating or take any offer ->

Buy BSQ 1

-> click "BUY BSQ" ->

Buy BSQ popup

-> click "BUY BSQ" in popup ->

image

Situations when the button is clicked (unchecked are still buggy):

  • user is creating/taking offer to BUY BTC, and under SELL BTC the tab "CREATE OFFER" is not active (it may be open but not selected);
  • user is creating/taking offer to BUY BTC, and under SELL BTC the active tab is "CREATE OFFER";
  • user is creating offer to SELL BTC;
  • user is taking offer to SELL BTC.

TODOs from discussion below:

  • close the current tab instead of just navigating away from it to the Offer Book;
  • don't display the button when the BSQ balance is already high enough.

Known bug:

  • Certain combination of the source and destination views may cause the "Filter by currency" combo box to receive focus, clearing it as a result. At that point, the value "BSQ" is still in effect, but any user action after that clears the filter. (More on that in the comments below.)

Discussion:

  1. If CREATE OFFER to SELL BTC is already open, user will be unable to create an offer, as the button CREATE NEW OFFER TO BUY BSQ will be disabled. There is no indication why. A tooltip explaining why it's disabled may be helpful.
  2. Should the popup include the "don't remind me again" option?
  3. Should we hide the button when we're creating/taking an offer to trade BTC for BSQ already?
  4. Having to create an altcoin account for BSQ seems like an unnecessary step. Can we have a built-in BSQ account, ideally one that will update the address to the next unused one dynamically?

@Conza88
Copy link

Conza88 commented Jan 5, 2021

Can we please add link to wiki on bsq etc?
So folks can easily see trading value of purchasing etc

@Jakub-CZ
Copy link
Contributor Author

Jakub-CZ commented Jan 5, 2021

I've been trying to resolve the combo box focus issue for a long time, with not much success. I tried the following:

  • disable the combo box temporarily,
  • disable the focus listener of the combo box temporarily, or setFocusTraversable(false),
  • requestFocus() on another element, right around the time the tab switching takes place (this resolved only some cases, mostly the 2nd item in my checkbox list above),
  • and much more.

These either had no effect, or caused the combo box not to reflect sellScreenCurrencyCode in Preferences.

Also, when the combo box receives focus, it's probably happening too early, as indicated by the fact that the drop-down list is rendered incorrectly - it's shifted to the left:

Buy BSQ tab select - combobox focus bug

Some options that I see:

  • carefuly rework AutocompleteComboBox. Instead of relying on onFocus, use onClick; I tried that already, but then the value in the combo box wasn't in line with sellScreenCurrencyCode in Preferences.
  • instead of clearing the value when focused, just select the contents of the edit text field; thus, the previous value will not be lost, and users can still very easily filter by typing right away.

@Jakub-CZ
Copy link
Contributor Author

Jakub-CZ commented Jan 5, 2021

Can we please add link to wiki on bsq etc?
So folks can easily see trading value of purchasing etc

This one? https://bisq.wiki/Trading_fees

Is this not enough?
image

@Conza88
Copy link

Conza88 commented Jan 5, 2021 via email

@pazza83
Copy link

pazza83 commented Jan 5, 2021

@Jakub-CZ looks great.

I had a read but not sure what happens when they click the 'Buy BSQ' button?

Are they taken to the Buy > BSQ/BTC market?

@Jakub-CZ
Copy link
Contributor Author

Jakub-CZ commented Jan 5, 2021

Yep, except it's Sell, as in "selling BTC for BSQ".

I've added the final screenshot in the description at the top.

@pazza83
Copy link

pazza83 commented Jan 5, 2021

Thanks, that is great. Hopefully will get more new users to buy some BSQ :)

@chimp1984
Copy link
Contributor

I had a quick look, but did not see anything obvious which could cause the problems you described. Would need to debug locally but too busy atm...

@Conza88
Copy link

Conza88 commented Jan 7, 2021

I really think these concerns need to be addressed before sending shiteload more people to buy bsq: #5046

@sqrrm
Copy link
Member

sqrrm commented Jan 13, 2021

For the case where someone is selling BTC I think the current sale tab needs to be closed and a new one opened for buying BSQ since there can only be one BTC sell tab open. That could perhaps be a bit confusing but I think allowing for multiple create offer tabs would be more trouble. Perhaps include a warning that the current BTC sell tab will be closed in the popup as well for that case.

Auto creating a BSQ account is reasonable and something I did for the not yet completed atomic BSQ trades. It's a bit more problematic for the current model as the receive address needs to be available throughout the trade process and our current model expects 1 single address per account. I think we could create a BSQ account with the first unused BSQ address if there is no other BSQ account. That would make it somewhat easier to get started with BSQ in the current state.

@Jakub-CZ I can't give any input on the UI elements not aligning and the focus, @ripcurlx knows that best I think.

Copy link
Member

@ripcurlx ripcurlx left a comment

Choose a reason for hiding this comment

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

I think having on the small green button Buy BSQ might confuse noobs what will happen if they click on this.
Maybe it should be something more like Get BSQ or How to get BSQ 👈 @m52go ?
We could use this popup afterwards not only to push the user to buy BSQ, but also show the possibility to earn BSQ by contributing.

Maybe we could also only show the Buy BSQ button if the trade has insufficient funds to pay the trading fee with BSQ.

Regarding the combobox question. So the goal is to open the offer book to sell BTC and have all BSQ offers selected? Atm it switches to the offer book setting the focus in the currency combobox.

For the case where someone is selling BTC I think the current sale tab needs to be closed and a new one opened for buying BSQ since there can only be one BTC sell tab open. That could perhaps be a bit confusing but I think allowing for multiple create offer tabs would be more trouble. Perhaps include a warning that the current BTC sell tab will be closed in the popup as well for that case.

That is the current behavior. It will just replace the current Take offer tab that was used for selling BTC in the first place. Maybe we just add a don't show again popup if a user is trying to take different offers to reduce confusion where the old take offer tab vanished to.

String buyBsqText = Res.get("shared.buyCurrency", "BSQ");
var buyBsqButton = new AutoTooltipButton(buyBsqText);
buyBsqButton.getStyleClass().add("action-button");
buyBsqButton.setStyle("-fx-pref-height: 20; -fx-padding: 3 8 3 8; -fx-font-size: 0.769em; -fx-border-radius: 5;");
Copy link
Member

Choose a reason for hiding this comment

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

Please move this into the css file and use another class instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@Jakub-CZ
Copy link
Contributor Author

@sqrrm
I thought about closing the tab as well, but it might be annoying to the user if they already filled in some of the fields (the button doesn't appear until at least the volume is filled in). Adding

Also, closing the tab may get around the UI issue I'm having right now.


@ripcurlx

Regarding the combobox question. So the goal is to open the offer book to sell BTC and have all BSQ offers selected? Atm it switches to the offer book setting the focus in the currency combobox.

Yes, that is the goal. The fact that it focuses on the currency combobox is an unintended side effect of the current design. Looks like the Offer Book view gets invalidated as a result of switching from Take Offer. I don't know why the same doesn't happed when switching from BUY BTC.

only show the Buy BSQ button if the trade has insufficient funds

Good idea. Will look into that.

Copy link
Contributor

@wallclockbuilder wallclockbuilder left a comment

Choose a reason for hiding this comment

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

UtAck

@Conza88
Copy link

Conza88 commented Mar 15, 2021

I think having on the small green button Buy BSQ might confuse noobs what will happen if they click on this.
Maybe it should be something more like Get BSQ or How to get BSQ 👈 @m52go ?
We could use this popup afterwards not only to push the user to buy BSQ, but also show the possibility to earn BSQ by contributing.

I think it may certainly confuse any noobs, and even old crew. Also it's like a check-out, instead of one-click amazon - you're putting up barriers for people to think about before making the trade. All else being equal, they'll likely be less inclined to go through with it. Less trades, not good for Bisq.

If the UX process was smooth for buying BSQ, I'd be less hesitant. Definitely not when it's a battle!

Shifting to a (i) How to get BSQ would not be a barrier to the final trade, but an FYI for future purposes.

@cd2357
Copy link
Contributor

cd2357 commented May 10, 2021

@Jakub-CZ any updates or roadblocks? Been looking forward to this PR for a long time :)

@Jakub-CZ
Copy link
Contributor Author

@cd2357 I'll try to get back to this during the upcoming weekend.

@Jakub-CZ
Copy link
Contributor Author

Jakub-CZ commented May 23, 2021

Updates since last review:

  • added link to relevant wiki page;
  • button styling moved to a CSS file;
  • button only displayed when not enough BSQ funds to pay for the fee.

There are still buggy scenarios where the "Filter by currency" combo box gets cleared intentionally, as a result of receiving focus. Switching to BUY BTC and then back to SELL BTC activates the desired filter (BSQ).

This needs more debugging and fixing.

@cd2357
Copy link
Contributor

cd2357 commented May 24, 2021

Tried it out, looks promising.

You're right, it can get tricky with the closing of tabs, updating states of UI components, etc.

To keep things simple, it might be enough to not do any UI transitions and navigation to the "Sell BTC / Buy BSQ" view, but instead to just open the "Buy BSQ" popup. If the user is interested, he will follow the links and read into it. The popup's button could then link to the step-by-step guide on how to go about and buy some BSQ.

@stale
Copy link

stale bot commented Jun 23, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

 Conflicts:
	desktop/src/main/java/bisq/desktop/main/offer/bisq_v1/takeoffer/TakeOfferView.java
 Conflicts:
	desktop/src/main/java/bisq/desktop/main/offer/offerbook/OfferBookView.java
 Conflicts:
	desktop/src/main/java/bisq/desktop/main/account/content/altcoinaccounts/AltCoinAccountsDataModel.java
 Conflicts:
	desktop/src/main/java/bisq/desktop/main/offer/offerbook/OfferBookView.java
	desktop/src/main/java/bisq/desktop/main/offer/offerbook/OfferBookViewModel.java
	desktop/src/main/java/bisq/desktop/util/DisplayUtils.java
@ripcurlx ripcurlx marked this pull request as ready for review November 4, 2021 12:28
chimp1984
chimp1984 previously approved these changes Nov 5, 2021
Copy link
Contributor

@chimp1984 chimp1984 left a comment

Choose a reason for hiding this comment

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

utACK

navigation.navigateTo(MainView.class, SellOfferView.class, OfferBookView.class);
}).show());

final VBox buyBsqButtonVBox = new VBox(buyBsqButton);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why a VBox is created if there is only one element in it?
The final in local variables should not be used (I know much of old code have it as well, but current code style suggest to not use it as unnecessary verbose).

Copy link
Member

Choose a reason for hiding this comment

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

Good question - I inherited this part, but didn't looked in the exact necessity of this layout. I'll simplify this positioning.

@chimp1984
Copy link
Contributor

I think the "BSQ is colored coin..." can be removed then the truncating with smaller width is not happening.

Screenshot 2021-11-05 at 01 36 06

Those methods are a bit weird as they mix up buyer/seller sides (isBuyBsqOffer / !isBuyOffer):

public boolean isBuyBsqOffer() {
        return !isBuyOffer() && getOffer().getCurrencyCode().equals("BSQ");
    }

@ripcurlx
Copy link
Member

ripcurlx commented Nov 8, 2021

public boolean isBuyBsqOffer() {
return !isBuyOffer() && getOffer().getCurrencyCode().equals("BSQ");
}

Yes, this is a little bit confusing because of the fact that when you buy an altcoin you actually selling Bitcoin. I can add an additional comment to make this less confusing. But actually the naming of isBuyBsqOffer is accurate.

@ripcurlx
Copy link
Member

ripcurlx commented Nov 8, 2021

I think the "BSQ is colored coin..." can be removed then the truncating with smaller width is not happening.

Screenshot 2021-11-05 at 01 36 06

I'll adapt the scaling behavior so that the informational box will truncate first and within the informational box the longer informational text.

@chimp1984
Copy link
Contributor

Yes the buy/sell direction is always referred to BTC. Only in the UI we swap it for altcoins as its not intuitive otherwise.
Maybe use a different term instead of buy/sell?

@ripcurlx
Copy link
Member

ripcurlx commented Nov 8, 2021

I think the "BSQ is colored coin..." can be removed then the truncating with smaller width is not happening.
Screenshot 2021-11-05 at 01 36 06

I'll adapt the scaling behavior so that the informational box will truncate first and within the informational box the longer informational text.

I've changed the behavior so it is not truncating at 1280:
Bildschirmfoto 2021-11-08 um 10 53 55
and will look as follows with the smallest width possible:
Bildschirmfoto 2021-11-08 um 10 54 01

Copy link
Contributor

@chimp1984 chimp1984 left a comment

Choose a reason for hiding this comment

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

utACK

Copy link
Member

@ripcurlx ripcurlx left a comment

Choose a reason for hiding this comment

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

ACK - based on #5055 (review)

@ripcurlx ripcurlx merged commit 00d7913 into bisq-network:master Nov 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Trade Step 1: Add "click here to buy BSQ" link next to trade fee selector
9 participants