Buy Now button with Variations #304

1bigidea · 2013-03-21T22:32:39Z

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

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

JustinSainton · 2013-03-22T04:18:17Z

wpsc-includes/display.functions.php

+				$disabled,
+				esc_attr( $product_id ),
+				esc_url( $src ),
+				esc_attr( 'PayPal - The safer, easier way to pay online', 'wpsc' )


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

I presume that you are talking about $disabled

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

We need to escape the "disabled" attribute. And apparently the Paypal string has never used the right i18n function

Cleaner code

meloniq · 2013-03-22T08:52:08Z

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());


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

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

Since all other jQuery selectors using the jQuery (rather than $ shortcut), the two references to self have modified

JustinSainton · 2013-03-22T15:36:13Z

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 · 2013-03-22T22:29:22Z

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 -

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.

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 [...] ).

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

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 -

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.

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 [...] ).

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.

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 -

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.

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 [...] ).

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.

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.

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?

minor whitespace corrections

rather than via sprintf

JustinSainton · 2013-05-03T21:53:20Z

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

instinct · 2013-05-03T22:22:40Z

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 · 2013-05-11T06:38:03Z

Related: #422.

1bigidea · 2013-05-11T14:58:40Z

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 · 2014-03-19T18:22:28Z

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

Buy Now button with Variations

a53b53e

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 mentioned this pull request Mar 21, 2013

Buy Now button and products with Variations - uses parent product not variation #303

Closed

JustinSainton reviewed Mar 22, 2013
View reviewed changes

1bigidea added 2 commits March 21, 2013 21:35

Escape the things

1b2ec55

We need to escape the "disabled" attribute. And apparently the Paypal string has never used the right i18n function

Use Disabled function, clean up code

eebb411

Cleaner code

meloniq reviewed Mar 22, 2013
View reviewed changes

Change $ to jQuery for consistency

cafac86

Since all other jQuery selectors using the jQuery (rather than $ shortcut), the two references to self have modified

JustinSainton reviewed Mar 22, 2013
View reviewed changes

1bigidea added 2 commits March 22, 2013 16:34

Reduce the calls to wpsc_product_has_variations

61c0b2f

minor whitespace corrections

Include disabled() in the input string directly

5b3f28b

rather than via sprintf

ghost assigned JustinSainton May 11, 2013

JustinSainton modified the milestones: Future Release, 3.8.14 Mar 19, 2014

JustinSainton force-pushed the master branch from 9868144 to a97791b Compare November 28, 2016 23:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Buy Now button with Variations #304

Buy Now button with Variations #304

1bigidea commented Mar 21, 2013

JustinSainton Mar 22, 2013

1bigidea Mar 22, 2013

JustinSainton Mar 22, 2013

meloniq Mar 22, 2013

1bigidea Mar 22, 2013

JustinSainton commented Mar 22, 2013

JustinSainton Mar 22, 2013

1bigidea Mar 22, 2013

1bigidea Mar 22, 2013

JustinSainton Mar 22, 2013

JustinSainton Mar 22, 2013

JustinSainton commented May 3, 2013

instinct commented May 3, 2013

JustinSainton commented May 11, 2013

1bigidea commented May 11, 2013

JustinSainton commented Mar 19, 2014

Buy Now button with Variations #304

Are you sure you want to change the base?

Buy Now button with Variations #304

Conversation

1bigidea commented Mar 21, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JustinSainton commented Mar 22, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JustinSainton commented May 3, 2013

instinct commented May 3, 2013

JustinSainton commented May 11, 2013

1bigidea commented May 11, 2013

JustinSainton commented Mar 19, 2014