Buy Now button with Variations #304

Open
wants to merge 6 commits into
from

Projects

None yet

4 participants

@1bigidea
Contributor

This is a long standing problem. When the Buy Now button is output (a standalone form), it doesn't include any variation information. As a result, any changes in the variation are ignored. Worse, when you press the Buy Now button, it submits to Paypal with the value of the parent product including whatever price is stored on the product record.

This will require changes to:

wp-e-commerce.js
when updating on change in selected variance, update the
corresponding Buy Now button

display.functions.php -
include a hidden field in the Buy Now form to pass the variation
value(s) to the Add_to_cart

@1bigidea 1bigidea Buy Now button with Variations
wp-e-commerce.js
    when updating on change in selected variance, update the
corresponding Buy Now button

display.functions.php -
    include a hidden field in the Buy Now form to pass the variation
value(s) to the Add_to_cart
a53b53e
@JustinSainton JustinSainton and 1 other commented on an outdated diff Mar 22, 2013
wpsc-includes/display.functions.php
$src = _x( 'https://www.paypal.com/en_US/i/btn/btn_buynow_LG.gif', 'PayPal Buy Now Button', 'wpsc' );
$src = apply_filters( 'wpsc_buy_now_button_src', $src );
$classes = "wpsc-buy-now-form wpsc-buy-now-form-{$product_id}";
- $button_html = '<input class="wpsc-buy-now-button wpsc-buy-now-button-' . esc_attr( $product_id ) . '" type="image" name="submit" border="0" src=' . esc_url( $src ) . ' alt="' . esc_attr( 'PayPal - The safer, easier way to pay online', 'wpsc' ) . '" />';
+ $button_html = sprintf('<input %s class="wpsc-buy-now-button wpsc-buy-now-button-%s" type="image" name="submit" border="0" src="%s" alt="%s" />',
+ $disabled,
+ esc_attr( $product_id ),
+ esc_url( $src ),
+ esc_attr( 'PayPal - The safer, easier way to pay online', 'wpsc' )
@JustinSainton
JustinSainton Mar 22, 2013 Member

This should be esc_attr__(), since it's being translated.

@1bigidea
1bigidea Mar 22, 2013 Contributor

I presume that you are talking about $disabled

@JustinSainton
JustinSainton Mar 22, 2013 Member

Nope - talking about this - esc_attr( 'PayPal - The safer, easier way to pay online', 'wpsc' ) - that should be esc_attr__( 'PayPal - The safer, easier way to pay online', 'wpsc' )

And now that you mention it, we should be using the disabled() function for the $disabled bit.

So I'd remove the chunk above (L38-L-40), and replace the $disabled var on L46 with disabled( $has_variants, wpsc_product_has_variations( $product_id ), false ) - also, the code there is also assigning, not comparing.

http://codex.wordpress.org/Function_Reference/disabled

1bigidea added some commits Mar 22, 2013
@1bigidea 1bigidea Escape the things
We need to escape the "disabled" attribute. And apparently the Paypal
string has never used the right i18n function
1b2ec55
@1bigidea 1bigidea Use Disabled function, clean up code
Cleaner code
eebb411
@meloniq meloniq and 1 other commented on an outdated diff Mar 22, 2013
wpsc-core/js/wp-e-commerce.js
@@ -320,6 +321,9 @@ jQuery(document).ready(function ($) {
}
}
donation_price.val(response.numeric_price);
+
+ buynow.find('input[name="'+$(self).prop('name')+'"]').val($(self).val());
@meloniq
meloniq Mar 22, 2013 Contributor

Shouldn't this be jQuery and not dollar sign? Whole code above uses that way...

@1bigidea
1bigidea Mar 22, 2013 Contributor

No difference except stylistically - but a simple enough change since everything else uses jQuery.

@1bigidea 1bigidea Change $ to jQuery for consistency
Since all other jQuery selectors using the jQuery (rather than $
shortcut), the two references to self have modified
cafac86
@JustinSainton
Member

Yeah, $/jQuery doesn't matter at all. This entire file is slated to be refactored in the near future anyway, at which point we'll be properly using the $ namespace alias.

@JustinSainton JustinSainton and 1 other commented on an outdated diff Mar 22, 2013
wpsc-includes/display.functions.php
@@ -38,12 +38,26 @@ function wpsc_buy_now_button( $product_id, $replaced_shortcode = false ) {
$src = _x( 'https://www.paypal.com/en_US/i/btn/btn_buynow_LG.gif', 'PayPal Buy Now Button', 'wpsc' );
$src = apply_filters( 'wpsc_buy_now_button_src', $src );
$classes = "wpsc-buy-now-form wpsc-buy-now-form-{$product_id}";
- $button_html = '<input class="wpsc-buy-now-button wpsc-buy-now-button-' . esc_attr( $product_id ) . '" type="image" name="submit" border="0" src=' . esc_url( $src ) . ' alt="' . esc_attr( 'PayPal - The safer, easier way to pay online', 'wpsc' ) . '" />';
+ $button_html = sprintf('<input %s class="wpsc-buy-now-button wpsc-buy-now-button-%s" type="image" name="submit" border="0" src="%s" alt="%s" />',
+ disabled(wpsc_product_has_variations($product_id), true, false),
@JustinSainton
JustinSainton Mar 22, 2013 Member

Sorry to keep nit-picking! A couple things -

  1. Since we're calling this wpsc_product_has_variations() function below on L53, too, we should definitely assign it to the $has_variants var. That way, we're not calling it twice. The performance is mitigated a bit by the static caching within the function - but I'd still reason it's worth only calling once.
  2. Watch the whitespace :) Anywhere we're updating or adding new code, we should be following WP coding conventions. So here, we'd want disabled( $has_variants [...] ).
  3. Speaking of whitespace, when disabled() returns "disabled='disabled'", it does so with the preceding space (See http://core.trac.wordpress.org/browser/tags/3.5.1/wp-includes/general-template.php#L2278). So you could have '<input%s class [...]' - that way, we don't have the somewhat annoying phantom spaces in the HTML
@1bigidea
1bigidea Mar 22, 2013 Contributor

I don't mind the criticism other than re-doing what was done before. (e.g. my if( $has_variants = wpsc……) )

On Mar 22, 2013, at 3:29 PM, JustinSainton notifications@github.com wrote:

In wpsc-includes/display.functions.php:

@@ -38,12 +38,26 @@ function wpsc_buy_now_button( $product_id, $replaced_shortcode = false ) {
$src = _x( 'https://www.paypal.com/en_US/i/btn/btn_buynow_LG.gif', 'PayPal Buy Now Button', 'wpsc' );
$src = apply_filters( 'wpsc_buy_now_button_src', $src );
$classes = "wpsc-buy-now-form wpsc-buy-now-form-{$product_id}";

  •       $button_html = '<input class="wpsc-buy-now-button wpsc-buy-now-button-' . esc_attr( $product_id ) . '" type="image" name="submit" border="0" src=' . esc_url( $src ) . ' alt="' . esc_attr( 'PayPal - The safer, easier way to pay online', 'wpsc' ) . '" />';
    
  •       $button_html = sprintf('<input %s class="wpsc-buy-now-button wpsc-buy-now-button-%s" type="image" name="submit" border="0" src="%s" alt="%s" />',
    
  •           disabled(wpsc_product_has_variations($product_id), true, false),
    

Sorry to keep nit-picking! A couple things -

  1. Since we're calling this wpsc_product_has_variations() function below on L53, too, we should definitely assign it to the $has_variants var. That way, we're not calling it twice.
  2. Watch the whitespace :) Anywhere we're updating or adding new code, we should be following WP coding conventions. So here, we'd want disabled( $has_variants [...] ).
  3. Speaking of whitespace, when disabled() returns "disabled='disabled'", it does so with the preceding space (See http://core.trac.wordpress.org/browser/tags/3.5.1/wp-includes/general-template.php#L2278). So you could have '<input%s class [...]' - that way, we don't have the somewhat annoying phantom spaces in the HTML


Reply to this email directly or view it on GitHub.

@1bigidea
1bigidea Mar 22, 2013 Contributor

And while the "disabled" function returns a leading space, pushing the %s up against "input" requires more "thinking" that the extra space costs. It looks like we are "modifying" the input value.

I would argue that it works against maintainable code.

On Mar 22, 2013, at 3:29 PM, JustinSainton notifications@github.com wrote:

In wpsc-includes/display.functions.php:

@@ -38,12 +38,26 @@ function wpsc_buy_now_button( $product_id, $replaced_shortcode = false ) {
$src = _x( 'https://www.paypal.com/en_US/i/btn/btn_buynow_LG.gif', 'PayPal Buy Now Button', 'wpsc' );
$src = apply_filters( 'wpsc_buy_now_button_src', $src );
$classes = "wpsc-buy-now-form wpsc-buy-now-form-{$product_id}";

  •       $button_html = '<input class="wpsc-buy-now-button wpsc-buy-now-button-' . esc_attr( $product_id ) . '" type="image" name="submit" border="0" src=' . esc_url( $src ) . ' alt="' . esc_attr( 'PayPal - The safer, easier way to pay online', 'wpsc' ) . '" />';
    
  •       $button_html = sprintf('<input %s class="wpsc-buy-now-button wpsc-buy-now-button-%s" type="image" name="submit" border="0" src="%s" alt="%s" />',
    
  •           disabled(wpsc_product_has_variations($product_id), true, false),
    

Sorry to keep nit-picking! A couple things -

  1. Since we're calling this wpsc_product_has_variations() function below on L53, too, we should definitely assign it to the $has_variants var. That way, we're not calling it twice.
  2. Watch the whitespace :) Anywhere we're updating or adding new code, we should be following WP coding conventions. So here, we'd want disabled( $has_variants [...] ).
  3. Speaking of whitespace, when disabled() returns "disabled='disabled'", it does so with the preceding space (See http://core.trac.wordpress.org/browser/tags/3.5.1/wp-includes/general-template.php#L2278). So you could have '<input%s class [...]' - that way, we don't have the somewhat annoying phantom spaces in the HTML


Reply to this email directly or view it on GitHub.

@JustinSainton
JustinSainton Mar 22, 2013 Member

No criticism intended - just a few small tweaks and we're there! And what was done before e.g. my if( $has_variants = wpsc……) wasn't quite right - it was checking if ( [...] ) on an assignment ($x = [...] ) - rather than a comparison.

I do see, from time to time, some clever coding (usually with transients, probably because of the codex) to this effect -

if ( false !== ( $x = func() ) ) {

}

I can get behind that when readability isn't sacrificed, but that's not quite what was happening there.

@JustinSainton
JustinSainton Mar 22, 2013 Member

WRT disabled() - that makes sense. Looking through core, however, there's actually no instance anywhere of disabled() (or the other helper functions) being run through sprintf() or printf(). There is, however (especially with checked()), precedence for putting it directly next to the attribute.

So with that in mind, what I'd actually suggest is removing the placeholder for disabled() and putting that directly in the variable - next to wherever you want it, sans extra space. I think that keeps up with your note of maintainability/readability. And it eliminates the extra space.

That work for you?

@JustinSainton
Member

Slating this for 3.8.12., @1bigidea, can you rebase?

@instinct
Member
instinct commented May 3, 2013

Yikes I don't even know how / if this works on 3.9 --- do we want it to?

Sent from my iPhone

On 4/05/2013, at 9:53 AM, JustinSainton notifications@github.com wrote:

Slating this for 3.8.12.


Reply to this email directly or view it on GitHub.

@JustinSainton
Member

Related: #422.

@1bigidea
Contributor

I am going to try and deal with #422 this weekend

On May 10, 2013, at 11:38 PM, JustinSainton notifications@github.com wrote:

Related: #422.


Reply to this email directly or view it on GitHub.

@JustinSainton
Member

Punting to a future release. Would be awesome to fix it though.

@JustinSainton JustinSainton modified the milestone: Future Release, 3.8.14 Mar 19, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment