Fancy Notifications Module (take 2) #2038

Merged
merged 9 commits into from May 20, 2016

Projects

None yet

2 participants

@benhuson
Member
benhuson commented Jan 6, 2016

Supercedes the previous PR #2020

Acts as a good basis to build integration with the V2 Theme engine #2007

benhuson added some commits Jan 6, 2016
@benhuson benhuson Add Fancy Notifications module. 8546a06
@benhuson benhuson Use Fancy Notifications module.
ac4ac0e
@JustinSainton JustinSainton and 1 other commented on an outdated diff Jan 6, 2016
...omponents/fancy-notifications/fancy-notifications.php
+ if ( $return ) {
+ return $output;
+ }
+ echo $output;
+
+ }
+
+ /**
+ * Fancy Notification Content
+ *
+ * @param array $cart_messages Cart message.
+ * @return string Fancy notification content.
+ */
+ public static function fancy_notification_content( $cart_messages ) {
+
+ $siteurl = get_option( 'siteurl' );
@JustinSainton
JustinSainton Jan 6, 2016 Member

@benhuson Can we use site_url() here?

@benhuson
benhuson Jan 7, 2016 Member

Actually $siteurl is not used so can be removed.

@JustinSainton JustinSainton and 1 other commented on an outdated diff Jan 6, 2016
wpsc-components/theme-engine-v1/helpers/ajax.php
@@ -185,9 +185,9 @@ function wpsc_add_to_cart() {
$json_response = $json_response + $output;
- if ( is_numeric( $product_id ) && 1 == get_option( 'fancy_notifications' ) ) {
- $json_response['fancy_notification'] = str_replace( array( "\n", "\r" ), array( '\n', '\r' ), fancy_notification_content( $cart_messages ) );
- }
+ //if ( is_numeric( $product_id ) && 1 == get_option( 'fancy_notifications' ) ) {
+ // $json_response['fancy_notification'] = str_replace( array( "\n", "\r" ), array( '\n', '\r' ), fancy_notification_content( $cart_messages ) );
+ //}
@JustinSainton
JustinSainton Jan 6, 2016 Member

@benhuson Is there any reason we're maintaining commented-out code?

@benhuson
benhuson Jan 7, 2016 Member

No. Oops.

@JustinSainton JustinSainton and 1 other commented on an outdated diff Jan 6, 2016
wpsc-core/js/wp-e-commerce.js
- position:'fixed',
- left: (jQuery(window).width() - jQuery('#fancy_notification').outerWidth())/2,
- top: (jQuery(window).height() - jQuery('#fancy_notification').outerHeight())/2
- });
-
- jQuery('#fancy_notification').css("display", 'block');
- jQuery('#loading_animation').css("display", 'block');
- jQuery('#fancy_notification_content').css("display", 'none');
+/**
+ * Submit the fancy notifications forms.
+ *
+ * @deprecated Use WPEC_Fancy_Notifications.fancy_notification() instead.
+ *
+ * @param object parent_form Form element.
+ */
+function wpsc_fancy_notification( parent_form ) {
@JustinSainton
JustinSainton Jan 6, 2016 Member

This instance of parent_form - and all notable references to it - don't seem to ever be used?

@benhuson
benhuson Jan 7, 2016 Member

I left in for legacy purposes but yes, it doesn't seem to be used. I'll remove.

@JustinSainton JustinSainton and 1 other commented on an outdated diff Jan 6, 2016
...ponents/fancy-notifications/js/fancy-notifications.js
+ if ( $( '#fancy_notification' ).length > 0 ) {
+ $( '#loading_animation' ).css( 'display', 'none' );
+ }
+
+ },
+
+ /**
+ * Fancy Notification
+ *
+ * @param object parent_form Form element.
+ */
+ fancy_notification : function( parent_form ) {
+
+ var fancyNotificationEl = $( '#fancy_notification' );
+
+ if ( typeof( WPSC_SHOW_FANCY_NOTIFICATION ) == 'undefined' ) {
@JustinSainton
JustinSainton Jan 6, 2016 Member

Getting this from Scrutinizer: === was expected, but instead == was given.

@benhuson
benhuson Jan 7, 2016 Member

Yep. Should be ===

Also 'typeof' should not be implemented as a function I think. See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/typeof

@JustinSainton JustinSainton and 1 other commented on an outdated diff Jan 6, 2016
...ponents/fancy-notifications/js/fancy-notifications.js
+ $( '#loading_animation' ).css( 'display', 'none' );
+ }
+
+ },
+
+ /**
+ * Fancy Notification
+ *
+ * @param object parent_form Form element.
+ */
+ fancy_notification : function( parent_form ) {
+
+ var fancyNotificationEl = $( '#fancy_notification' );
+
+ if ( typeof( WPSC_SHOW_FANCY_NOTIFICATION ) == 'undefined' ) {
+ WPSC_SHOW_FANCY_NOTIFICATION = true;
@JustinSainton
JustinSainton Jan 6, 2016 Member

Also from Scrutinizer: WPSC_SHOW_FANCY_NOTIFICATION does not seem to be defined.

@benhuson
benhuson Jan 7, 2016 Member

I think the purpose of that JS variable is that if you define it and set it to false it will disable fancy notifications.

Again, kept for backwards compatibility.

If we defined it somewhere I don't think we could guarantee that someone may not have defined it before us?

Haven't checked if the admin setting is filterable at all. If not it probably should be and we could try to deprecate this use case?

@benhuson
Member
benhuson commented Jan 7, 2016

Will take a look at your comments.

If I add @since version to doc blocks, what version should I specify?

benhuson added some commits Jan 7, 2016
@benhuson benhuson Remove unused $siteurl from fancy_notification_content(). ba7be18
@benhuson benhuson Remove commented-out code in wpsc_add_to_cart(). 0d7a3a9
@benhuson benhuson Streamline code where `wpsc_add_to_cart_json_response` is used. 5948eb4
@benhuson benhuson Deprecate `parent_form` in WPEC_Fancy_Notifications.fancy_notificatio…
…n().
a5d336e
@benhuson benhuson Fix use of JavaScript `typeof` and use === in WPEC_Fancy_Notification…
…s.fancy_notification().
175e73f
@benhuson benhuson Remove `parent_form` in WPEC_Fancy_Notifications.fancy_notification().
0fd7e14
@JustinSainton
Member

@benhuson Using 4.0 as the since tag should be fine.

Looking good!

@benhuson benhuson Add Fancy Noifications module @since documentation.
75f1637
@JustinSainton JustinSainton merged commit ca99f84 into wp-e-commerce:master May 20, 2016

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
Scrutinizer 5 new issues, 9 updated code elements
Details
@JustinSainton JustinSainton added this to the 4.0 milestone May 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment