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

PoS: Custom buy button text per product #2299

Merged
merged 14 commits into from Feb 26, 2021

Conversation

dennisreimann
Copy link
Member

@dennisreimann dennisreimann commented Feb 18, 2021

Closes #2232.

I saw the bounty by @MaxHillebrand in the chat and did this – only realizing afterwards that @bolatovumar already discussed this with Max and wanted to take it on. As I didn't intend to get in the way I'd suggest Umar reviews this and we split the bounty if that's okay too.

Editor

Works with regular text too, this example uses the {0} to be replaced by the amount, which is supported on a store level too.

editor

Static View

static

Cart View

cart

Copy link
Member

@Kukks Kukks left a comment

Choose a reason for hiding this comment

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

Need to take into account crowdfund app too as it uses the same templating

@dennisreimann
Copy link
Member Author

@Kukks Thanks for the pointer, added.

crowdfund

@MaxHillebrand
Copy link
Contributor

You're a treasure @dennisreimann, thanks for picking it up so fast :)

Yes, I'm happy to include reviewers of the PR in the bounty.

Copy link
Member

@Kukks Kukks left a comment

Choose a reason for hiding this comment

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

LGTM, just a small quyestion if the prop name should be simpler or not but nothing major

@@ -17,6 +17,7 @@ public class ItemPrice
public ItemPrice Price { get; set; }
public string Title { get; set; }
public bool Custom { get; set; }
public string BuyButtonText { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

BuyButtonText or simply ButtonText?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought about this too and was torn: The upper level prop is called CustomButtonText – one might argue we should match this, but I wanted to avoid confusion and used the buy-prefix to be specific … so yeah, let's discuss this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think BuyButtonText is more accurate even though it's more verbose.

Copy link
Contributor

@pavlenex pavlenex left a comment

Choose a reason for hiding this comment

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

If custom price is set to yes the text you add in the button name field doesn't take effect. If it's desired behavior, we should probably hide the Buy Button Text field, if it's not desired, the name in the Buy Button Text should take effect even if custom price is set to yes.

Screenshot from 2021-02-19 17-23-46

Screenshot from 2021-02-19 17-26-22

@bolatovumar
Copy link
Contributor

If custom price is set to yes the text you add in the button name field doesn't take effect. If it's desired behavior, we should probably hide the Buy Button Text field, if it's not desired, the name in the Buy Button Text should take effect even if custom price is set to yes.

Screenshot from 2021-02-19 17-23-46

Screenshot from 2021-02-19 17-26-22

Yeah, I think if you specify custom button text it should apply no matter what you select for "Custom price". The user who puts in custom button text probably doesn't care what is selected in the "Custom price" dropdown when they set the button text, they just want this text to appear no matter what.

@dennisreimann
Copy link
Member Author

Fixed, also updated the display so that it works with long and short text …

custom-price

Copy link
Contributor

@pavlenex pavlenex left a comment

Choose a reason for hiding this comment

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

tACK.

@pavlenex pavlenex added this to In progress in 0.12 Milestone via automation Feb 22, 2021
@pavlenex pavlenex moved this from In progress to Review in progress in 0.12 Milestone Feb 22, 2021
@pavlenex pavlenex moved this from Review in progress to Reviewer approved in 0.12 Milestone Feb 22, 2021
Copy link
Contributor

@bolatovumar bolatovumar left a comment

Choose a reason for hiding this comment

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

I tested this branch locally on my machine and noticed a couple of issues:

  1. If I add a custom text to the first item in the default POS app (green tea) and then update it again right after all OTHER items get the old text for the first item after the second update (see screenshot). I can reproduce this consistently.
    • Screen Shot 2021-02-22 at 6 59 50 PM
  2. If I add text that's too long the button overspills. This one is probably not a big deal as I don't expect people will add long text in these buttons
    • Screen Shot 2021-02-22 at 7 01 36 PM

@dennisreimann
Copy link
Member Author

@bolatovumar good spot, it's fixed now. As far as I read it, we had the same problem before with other properties (price, title, description and custom) because they weren't reset before parsing a new item. Fixed in 3237054.

Copy link
Contributor

@bolatovumar bolatovumar left a comment

Choose a reason for hiding this comment

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

Tested the updated branch and couldn't find any issues.

@@ -478,12 +478,21 @@ public async Task CanCreateAppPoS()
s.Driver.FindElement(By.Id("SelectedStore")).SendKeys(storeName);
s.Driver.FindElement(By.Id("Create")).Click();
s.Driver.FindElement(By.Id("DefaultView")).SendKeys("Cart");
s.Driver.FindElement(By.CssSelector(".template-item:nth-of-type(1) .btn-primary")).Click();
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be better to use some test-id attribute instead so if a new button is added or something you don't have to update this test?

Copy link
Member Author

Choose a reason for hiding this comment

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

In general I agree. This one selects the first article in the editor – as it is a dynamic list I thought it'd be ok this way too.

<div class="form-row">
<div class="col">
<label>Buy Button Text</label>
<input type="text" id="BuyButtonText" class="form-control mb-2" v-model="editingItem.buyButtonText" ref="txtBuyButtonText"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

ref attribute seems weird. txtBuyButtonText <-- looks like txt and Text are meant to represent text and are repeated?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's the same as for the other input fields – I think the txt part refers to the input being a text input. Can change it, but think we should then change the other field refs too to be consistent.

@dennisreimann
Copy link
Member Author

Fix for the Selenium test contained in #2310. Can rebase this one if the other PR gets merged first.

Co-authored-by: Max Hillebrand <30683012+MaxHillebrand@users.noreply.github.com>
@dennisreimann dennisreimann added Enhancement Improvements to an existing feature PoS Point of Sale app labels Feb 24, 2021
@NicolasDorier NicolasDorier merged commit 6d9b93a into btcpayserver:master Feb 26, 2021
0.12 Milestone automation moved this from Reviewer approved to Done Feb 26, 2021
@dennisreimann dennisreimann deleted the pos-item-button-text branch February 26, 2021 07:10
@MaxHillebrand
Copy link
Contributor

Thank you @dennisreimann @bolatovumar @Kukks @pavlenex for all you past and present support of free software like BTCPay! Y'all are heroes!!

Some sats have magically found their way into your wallets. 🖤 🔥

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Improvements to an existing feature PoS Point of Sale app
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

7 participants