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.
>