Migrate over GEM notice support #21

EvanHerman · 2016-12-08T18:23:46Z

@jonathanbardo As discussed this is migrated over from the system plugin branch.

~~New functions and files have been introduced, the version has been bumped to 1.1.4 and the readme.txt has been updated accordingly~~.

Update:
An informational notice is displayed back to the user on the initial dashboard page load, and then wpem_gem_notice option is deleted from the database to prevent subsequent page loads.

Note: Needs translations.

Original PR:

Display an admin notice with information about the GEM plugin on fresh WordPress installs.

This is a persistent notice, with the ability to dismiss. Once the 'x' has been clicked, the notice will no longer appear back to the user.

The .pot file needs to be updated with the new strings.

jonathanbardo · 2016-12-08T20:24:29Z

includes/settings.php

@@ -545,15 +545,6 @@ public function display_settings_page() {

 				<?php if ( ! empty( $forms->signups ) ) : ?>
 					<div id="forms" class="panel">
-						<h3><?php esc_html_e( 'Reach Your Fans', 'godaddy-email-marketing' ); ?></h3>


@EvanHerman Why did you remove that?

jonathanbardo · 2016-12-08T20:26:51Z

godaddy-email-marketing.php

+		add_action( 'init',                            array( $this, 'register_shortcode' ), 20 );
+		add_Action( 'admin_enqueue_scripts',           array( $this, 'register_admin_scripts' ) );
+		add_action( 'admin_notices',                   array( $this, 'action_admin_notices' ) );
+		add_action( 'wp_ajax_dismiss_gem_notice',      array( $this, 'delete_wpem_gem_notice_option' ) );


@EvanHerman We can reduce the spacing between the filter name and the array. Only 1 space between the comman of the biggest filter name.

jonathanbardo · 2016-12-08T20:27:30Z

godaddy-email-marketing.php

+		add_action( 'init',                            array( $this, 'init' ) );
+		add_action( 'widgets_init',                    array( $this, 'register_widget' ) );
+		add_action( 'init',                            array( $this, 'register_shortcode' ), 20 );
+		add_Action( 'admin_enqueue_scripts',           array( $this, 'register_admin_scripts' ) );


@EvanHerman I think there is a typo in add_Action.

jonathanbardo · 2016-12-08T20:30:11Z

godaddy-email-marketing.php

+	 */
+	public function register_admin_scripts() {
+
+		if ( get_option( 'wpem-gem-notice' ) ) {


@EvanHerman Might be better to use an early return statement here.

if ( ! get_option( 'wpem-gem-notice' ) ) { return; }

jonathanbardo · 2016-12-08T20:34:04Z

godaddy-email-marketing.php

 	 * Displays the admin notice.
 	 */
 	public function action_admin_notices() {
+
+		if ( get_option( 'wpem-gem-notice' ) ) {


@EvanHerman Let's add a private function to get rid of this if. $this->display_gem_notice or something that makes sense.

jonathanbardo · 2016-12-08T20:36:01Z

godaddy-email-marketing.php

+	/**
+	 * Delete the wpem-gem-notice option from the database
+	 *
+	 * @return boolean


@EvanHerman This function doesn't return a boolean.

jonathanbardo · 2016-12-08T20:39:02Z

godaddy-email-marketing.php

+					<?php
+						printf(
+							__( 'Your website has a superpower: Email marketing. %1$s.', 'gd-system-plugin' ),
+							'<a href="' . admin_url( 'options-general.php?page=gem-settings' ) . '">' . __( 'Learn More', 'gd-system-plugin' ) . '</a>'


@EvanHerman The text domain is not the right one.

frankiejarrett · 2016-12-08T20:06:50Z

js/gem-notice.js

@@ -0,0 +1,15 @@
+/**


@EvanHerman Do we even need this file? I see we're using the .is-dismissible class already on the notice. Does the dismiss functionality not work out-of-the-box?

cf. https://make.wordpress.org/core/2015/04/23/spinners-and-dismissible-admin-notices-in-4-2/

@fjarrett I think we do since .is-dismissible only adds the X and the animation and nothing else.

We need to fire the ajax somehow so the alert doesn't come back

The dismiss function works out of the box, but it only hides the container. The option will still exist in the database causing the notice to display again.

This file fires an AJAX function that removes the option from the database, preventing the notice from displaying on subsequent page loads.

I don't like that we have to do so much for this. Seems like it should be simpler...

@jonathanbardo @EvanHerman why not just delete the wpem_gem_notice_option on shutdown if we know it exists? Then the message would fire only once the first time you go to the Admin, and go away with a click of the X or reload of any page.

@fjarrett We can go that route, but I was worried that the notice would get overlooked after the first page load. It is less code that route. If you think that would be best I can adjust the PR accordingly.

@EvanHerman I say yes because it seems to still meet the requirements and is way less code to maintain. @jonathanbardo What do you think?

frankiejarrett · 2016-12-08T20:08:12Z

includes/settings.php

-								'</a>'
-							);
-							?>
-						</p>


@EvanHerman Was removing this text from the Settings page part of the scope?

@fjarrett No, I think I originally forked this from another branch that didn't yet have this block of code.

frankiejarrett · 2016-12-08T22:54:21Z

godaddy-email-marketing.php

@@ -151,6 +158,7 @@ public function init() {

 		// Initialize settings.
 		if ( is_admin() ) {
+			$this->display_gem_notice = get_option( 'wpem_gem_notice' );


@EvanHerman Seems like we should be able to look for this inside the action_admin_notices method and don't need this private property at all. Or did you already try that?

@fjarrett You're right. This was originally placed in a private object because it was used in more than one location (DRY). Without the AJAX this is un-necessary. Removing it now.

frankiejarrett · 2016-12-08T23:21:31Z

godaddy-email-marketing.php

+			</div>
+
+			<?php
+


@EvanHerman I say we just delete the option right here after displaying the notice.

coveralls · 2016-12-09T06:41:08Z

Coverage decreased (-1.0%) to 98.845% when pulling a1ff00a on gem-notice into 5b36b6d on develop.

Migrate gem notice support over.

a24fda4

EvanHerman added the enhancement label Dec 8, 2016

EvanHerman added 4 commits December 8, 2016 13:26

Remove p tags from printf().

866fba7

Add missing docblock and @SInCE tags.

00a07fb

Localize ajaxurl.

9c4d522

Remove ajax_url localization.

9ab60db

frankiejarrett assigned EvanHerman Dec 8, 2016

frankiejarrett self-requested a review December 8, 2016 20:08

jonathanbardo reviewed Dec 8, 2016

View reviewed changes

EvanHerman added 3 commits December 8, 2016 15:34

re-add missing settings section.

5a66db4

Revert version bump.

2d885e9

Update array syntax.

cf9cd8c

jonathanbardo requested changes Dec 8, 2016

View reviewed changes

jonathanbardo reviewed Dec 8, 2016

View reviewed changes

EvanHerman added 4 commits December 8, 2016 15:45

Fix spacing. Introduce private variable. Fix text domain.

337bc02

Add global variable atop js file.

4bd56ea

Fix global variable spacing.

97eec92

Fix global variable spacing.

74602b0

frankiejarrett reviewed Dec 8, 2016

View reviewed changes

EvanHerman added 5 commits December 8, 2016 17:09

Strip out ajax functionality.

fbb983a

Spacing fix.

6ef15c4

Update unit tests.

b1a660d

Update option name.

8aa56d9

Add missing private variable definition.

f332dec

frankiejarrett reviewed Dec 8, 2016

View reviewed changes

Remove object, shift get_option().

ab6ee5a

frankiejarrett reviewed Dec 8, 2016

View reviewed changes

EvanHerman and others added 2 commits December 8, 2016 18:24

Delete wpem_gem_notice option once the notice is displayed.

f983014

Merge in develop

a1ff00a

frankiejarrett closed this Dec 8, 2016

frankiejarrett deleted the gem-notice branch December 8, 2016 23:48

frankiejarrett mentioned this pull request Dec 8, 2016

Add notice after WPEM #22

Merged

EvanHerman mentioned this pull request Jan 19, 2017

Merge develop into master #25

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate over GEM notice support #21

Migrate over GEM notice support #21

EvanHerman commented Dec 8, 2016 •

edited

Loading

jonathanbardo Dec 8, 2016

jonathanbardo Dec 8, 2016

jonathanbardo Dec 8, 2016

jonathanbardo Dec 8, 2016

jonathanbardo Dec 8, 2016

jonathanbardo Dec 8, 2016

jonathanbardo Dec 8, 2016

frankiejarrett Dec 8, 2016

jonathanbardo Dec 8, 2016

jonathanbardo Dec 8, 2016

EvanHerman Dec 8, 2016

frankiejarrett Dec 8, 2016

frankiejarrett Dec 8, 2016

EvanHerman Dec 8, 2016

frankiejarrett Dec 8, 2016

frankiejarrett Dec 8, 2016

EvanHerman Dec 8, 2016

frankiejarrett Dec 8, 2016

EvanHerman Dec 8, 2016 •

edited

Loading

frankiejarrett Dec 8, 2016

coveralls commented Dec 9, 2016 •

edited

Loading

Migrate over GEM notice support #21

Migrate over GEM notice support #21

Conversation

EvanHerman commented Dec 8, 2016 • edited Loading

Original PR:

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EvanHerman Dec 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Dec 9, 2016 • edited Loading

EvanHerman commented Dec 8, 2016 •

edited

Loading

EvanHerman Dec 8, 2016 •

edited

Loading

coveralls commented Dec 9, 2016 •

edited

Loading