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.

Sign up for GitHub

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

Buy Now button with Variations #304

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Buy Now button with Variations #304

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a53b53e
Buy Now button with Variations
1bigidea Mar 21, 2013
1b2ec55
Escape the things
1bigidea Mar 22, 2013
eebb411
Use Disabled function, clean up code
1bigidea Mar 22, 2013
cafac86
Change $ to jQuery for consistency
1bigidea Mar 22, 2013
61c0b2f
Reduce the calls to wpsc_product_has_variations
1bigidea Mar 22, 2013
5b3f28b
Include disabled() in the input string directly
1bigidea Mar 22, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion wpsc-core/js/wp-e-commerce.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,6 +280,7 @@ jQuery(document).ready(function ($) {
// update the price when the variations are altered.
jQuery(".wpsc_select_variation").live('change', function() {
jQuery('option[value="0"]', this).attr('disabled', 'disabled');
self = this;
var parent_form = jQuery(this).closest("form.product_form");
if ( parent_form.length == 0 )
return;
Expand All @@ -292,7 +293,7 @@ jQuery(document).ready(function ($) {
donation_price = jQuery('input#donation_price_' + prod_id),
old_price = jQuery('#old_product_price_' + prod_id),
save = jQuery('#yousave_' + prod_id),
buynow = jQuery('#BB_BuyButtonForm' + prod_id);
buynow = jQuery('#buy-now-product_' + prod_id);
if ( response.variation_found ) {
if ( response.stock_available ) {
stock_display.removeClass('out_of_stock').addClass('in_stock');
Expand Down Expand Up @@ -320,6 +321,9 @@ jQuery(document).ready(function ($) {
}
}
donation_price.val(response.numeric_price);

buynow.find('input[name="'+$(self).prop('name')+'"]').val($(self).val());
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

buynow.find('input.wpsc-buy-now-button').prop('disabled', false);
}
}, 'json');
});
Expand Down
26 changes: 20 additions & 6 deletions wpsc-includes/display.functions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

esc_attr( $product_id ),
esc_url( $src ),
esc_attr__( 'PayPal - The safer, easier way to pay online', 'wpsc' )
);
$button_html = apply_filters( 'wpsc_buy_now_button_html', $button_html, $product_id );
?>
<form class="<?php echo esc_attr( $classes ); ?>" target="paypal" action="<?php echo esc_url( home_url() ); ?>" method="post">
?>
<form class="<?php echo esc_attr( $classes ); ?>" id="buy-now-product_<?php echo $product_id; ?>"target="paypal" action="<?php echo esc_url( home_url() ); ?>" method="post">
<input type="hidden" name="wpsc_buy_now_callback" value="1" />
<input type="hidden" name="product_id" value="<?php echo esc_attr( $product_id ); ?>" />
<?php
if( wpsc_product_has_variations($product_id) ):
// grab the variation form fields here
$wpsc_variations = new wpsc_variations( $product_id );
while ( wpsc_have_variation_groups() ) : wpsc_the_variation_group();
printf('<input type="hidden" class="variation-value" name="variation[%s]" id="%s" value="0"/>', wpsc_vargrp_id(), wpsc_vargrp_form_id());
endwhile;
endif; /* END wpsc_product_has_variations */
?>
<?php if ( get_option( 'multi_add' ) ): ?>
<label for="quantity"><?php esc_html_e( 'Quantity', 'wpsc' ); ?></label>
<input type="text" size="4" id="quantity" name="quantity" value="" /><br />
Expand All @@ -70,20 +84,20 @@ function wpsc_also_bought( $product_id ) {
if ( get_option( 'wpsc_also_bought' ) == 0 ) {
return '';
}

// To be made customiseable in a future release
$also_bought_limit = 3;
$element_widths = 96;
$image_display_height = 96;
$image_display_width = 96;

// Filter will be used by a plugin to provide 'Also Bought' functionality when this is deprecated from core.
// Filter is currently private and should not be used by plugin/theme devs as it may only be temporary.
$output = apply_filters( '_wpsc_also_bought', '', $product_id );
if ( ! empty( $output ) ) {
return $output;
}

// If above filter returns output then the following is ignore and can be deprecated in future.
$also_bought = $wpdb->get_results( $wpdb->prepare( "SELECT `" . $wpdb->posts . "`.* FROM `" . WPSC_TABLE_ALSO_BOUGHT . "`, `" . $wpdb->posts . "` WHERE `selected_product`= %d AND `" . WPSC_TABLE_ALSO_BOUGHT . "`.`associated_product` = `" . $wpdb->posts . "`.`id` AND `" . $wpdb->posts . "`.`post_status` IN('publish','protected') ORDER BY `" . WPSC_TABLE_ALSO_BOUGHT . "`.`quantity` DESC LIMIT $also_bought_limit", $product_id ), ARRAY_A );
if ( is_array( $also_bought ) && count( $also_bought ) > 0 ) {
Expand Down