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.

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

Display a welcome notice on the main 'AMP Settings' page #1442

Merged
merged 8 commits into from Sep 19, 2018

Conversation

Projects
None yet
3 participants
@kienstra
Collaborator

kienstra commented Sep 18, 2018

  • Adds a placeholder notice to the 'AMP Settings' page, until the copy is available
  • Once the user dismisses this, it will never appear again

amp-settings-notice

Closes #1381

Output a placeholder notice on the main 'AMP Settings' page
@todo: Add the full copy for this notice when it is decided.
Once this notice is dismissed,
it won't display again.

@kienstra kienstra referenced this pull request Sep 18, 2018

Closed

Implement settings splash notice #1381

6 of 6 tasks complete
Move welcome notice above persistent object caching notice
That notice is important also,
but the welcome notice will only appear until
it is dismissed.
$notice_id = 'welcome-notice-1';
$dismissed = get_user_meta( get_current_user_id(), 'dismissed_wp_pointers', true );
if ( in_array( $notice_id, explode( ',', strval( $dismissed ) ), true ) ) {

This comment has been minimized.

@kienstra

kienstra Sep 18, 2018

Collaborator

Question About Admin Pointer User Meta

Hi @westonruter,
What do you think about how this uses the user meta for admin pointers to keep track of whether this notice was dismissed?

This is very similar to how this admin pointer PR does this.

Of course, this isn't actually an admin pointer, so it's not ideal.

But this is a little simpler, as it can reuse the dismiss-wp-pointer action handler when the user dismisses it.

Thanks, Weston!

This comment has been minimized.

@westonruter

westonruter Sep 18, 2018

Member

My main concern is what happens over time. If each new version of the plugin will include new information that is shown in this message, then the dismissed_wp_pointers usermeta will grow endlessly.

You could instead consider what was added for the Try Gutenberg screen:

https://github.com/WordPress/wordpress-develop/blob/5d477aa70613850a2e78cdaecb7e2de365de7134/src/wp-admin/js/dashboard.js#L34-L75

https://github.com/WordPress/wordpress-develop/blob/5d477aa70613850a2e78cdaecb7e2de365de7134/src/wp-admin/includes/ajax-actions.php#L1487-L1498

But maybe this is all overkill. For a one-off, I don't see a reason why to not use the admin-pointers. They're being used for non-pointer things already, including the theme and plugin editor warnings:

https://github.com/WordPress/wordpress-develop/blob/5d477aa70613850a2e78cdaecb7e2de365de7134/src/wp-admin/plugin-editor.php#L282-L294

https://github.com/WordPress/wordpress-develop/blob/5d477aa70613850a2e78cdaecb7e2de365de7134/src/wp-admin/theme-editor.php#L301-L313

https://github.com/WordPress/wordpress-develop/blob/5d477aa70613850a2e78cdaecb7e2de365de7134/src/wp-admin/js/theme-plugin-editor.js#L134-L150

So I'd say go ahead with pointers.

Address Travis error by storing ob_get_clean() in variable
When it was called for the second time,
it was empty.
So simply store it in a variable.
return;
}
$notice_id = 'welcome-notice-1';

This comment has been minimized.

@westonruter

westonruter Sep 18, 2018

Member

This should have some amp prefix.

This comment has been minimized.

@kienstra

kienstra Sep 18, 2018

Collaborator

Good point. 8f02acf#diff-6dc720c5e5352d31e656ef4973984528R344 makes this amp-welcome-notice-1

( function( $ ) {
$( function() {
// On dismissing the notice, make a POST request to store this notice with the dismissed WP pointers so it doesn't display again.
$( document ).on( 'click', '#<?php echo $notice_id; // WPCS: XSS OK. ?> .notice-dismiss', function() {

This comment has been minimized.

@westonruter

westonruter Sep 18, 2018

Member

Instead of: '#<?php echo $notice_id; // WPCS: XSS OK. ?> .notice-dismiss'
Do this: <?php echo wp_json_encode( "#$notice_id .notice-dismiss" ); ?>

This comment has been minimized.

@kienstra

kienstra Sep 18, 2018

Collaborator

Thanks, 8f02acf#diff-6dc720c5e5352d31e656ef4973984528R357 applies your suggestion.

$( document ).on( 'click', '#<?php echo $notice_id; // WPCS: XSS OK. ?> .notice-dismiss', function() {
console.log( 'clicked' );
$.post( ajaxurl, {
pointer: '<?php echo $notice_id; // WPCS: XSS OK. ?>',

This comment has been minimized.

@westonruter

westonruter Sep 18, 2018

Member

As above, better to do pointer: <?php echo wp_json_encode( $notice_id ); ?>,

This comment has been minimized.

@kienstra
$notice_id = 'welcome-notice-1';
$dismissed = get_user_meta( get_current_user_id(), 'dismissed_wp_pointers', true );
if ( in_array( $notice_id, explode( ',', strval( $dismissed ) ), true ) ) {

This comment has been minimized.

@westonruter

westonruter Sep 18, 2018

Member

My main concern is what happens over time. If each new version of the plugin will include new information that is shown in this message, then the dismissed_wp_pointers usermeta will grow endlessly.

You could instead consider what was added for the Try Gutenberg screen:

https://github.com/WordPress/wordpress-develop/blob/5d477aa70613850a2e78cdaecb7e2de365de7134/src/wp-admin/js/dashboard.js#L34-L75

https://github.com/WordPress/wordpress-develop/blob/5d477aa70613850a2e78cdaecb7e2de365de7134/src/wp-admin/includes/ajax-actions.php#L1487-L1498

But maybe this is all overkill. For a one-off, I don't see a reason why to not use the admin-pointers. They're being used for non-pointer things already, including the theme and plugin editor warnings:

https://github.com/WordPress/wordpress-develop/blob/5d477aa70613850a2e78cdaecb7e2de365de7134/src/wp-admin/plugin-editor.php#L282-L294

https://github.com/WordPress/wordpress-develop/blob/5d477aa70613850a2e78cdaecb7e2de365de7134/src/wp-admin/theme-editor.php#L301-L313

https://github.com/WordPress/wordpress-develop/blob/5d477aa70613850a2e78cdaecb7e2de365de7134/src/wp-admin/js/theme-plugin-editor.js#L134-L150

So I'd say go ahead with pointers.

kienstra added some commits Sep 18, 2018

Prefix notice ID, use wp_json_encode to in script
On Weston's suggestion,
as this is more secure.
Replace the 'Settings saved' notice the first time the user saves som…
…ething

The copy can still change,
but this now has Leo's copy for all 3 template modes.
Revert "Replace the 'Settings saved' notice the first time the user s…
…aves something"

This is intended for Issue 1399,
and I meant to commit this to a separate branch for that.
This reverts commit 3ec5334.
( function( $ ) {
$( function() {
// On dismissing the notice, make a POST request to store this notice with the dismissed WP pointers so it doesn't display again.
$( document ).on( 'click', <?php echo wp_json_encode( "#$notice_id .notice-dismiss" ); ?>, function() {

This comment has been minimized.

@westonruter

westonruter Sep 18, 2018

Member

Another point, this code is using event delegation when it doesn't need to. It should instead just be:

$( <?php echo wp_json_encode( "#$notice_id .notice-dismiss" ); ?> ).on( 'click', function() {...});

This comment has been minimized.

@kienstra

kienstra Sep 18, 2018

Collaborator

Hi Weston, good point.

But it looks like the button.notice-dismiss might be created with JS when the is-dismissible class is present, and might not yet exist at the time the <script> above runs.

<button type="button" class="notice-dismiss">

So the click handler with that selector doesn't run.

Maybe if this script ran later, the button.notice-dismiss would be present.

This comment has been minimized.

@westonruter

westonruter Sep 18, 2018

Member

ok, that makes sense

This comment has been minimized.

@westonruter

westonruter Sep 19, 2018

Member

At least we can do this:

$( <?php echo wp_json_encode( "#$notice_id" ); ?> ).on( 'click', '.notice-dismiss', function() {

That will scope the delegated event to the containing element.

Add the message copy from Issue 1381
Also, add an icon in the top left.
And copy the styling for this icon,
slightly modifiying it.
Like the margin: 5px;
@kienstra

This comment has been minimized.

Collaborator

kienstra commented Sep 19, 2018

Notice Copy Applied
6e9a09e applies the notice copy from #1381 (comment):

notice-amp

<span class="amp-logo-icon"></span>
<h1><?php esc_html_e( 'Welcome to the AMP for WordPress plugin v1.0', 'amp' ); ?></h1>
<p><?php esc_html_e( 'Thank you for installing! Bring the speed and features of the open source AMP project to your site, the WordPress way, complete with the tools to support content authoring and website development.', 'amp' ); ?></p>
<h2><?php esc_html_e( 'What&#39;s New', 'amp' ); ?></h2>

This comment has been minimized.

@kienstra

kienstra Sep 19, 2018

Collaborator

The &#39; character here might not be the best way to render the '

This comment has been minimized.

@westonruter

westonruter Sep 19, 2018

Member

That's right. It should be &#8217; instead.

@kienstra

This comment has been minimized.

Collaborator

kienstra commented Sep 19, 2018

Request For Another Round Of Review

Hi @westonruter,
Could you please have another look at this PR, especially the latest commit that applies the notice copy?

Thanks, Weston!

@kienstra

This comment has been minimized.

Collaborator

kienstra commented Sep 19, 2018

'Thank you for installing!' text

Ideally, this would display 'Thank you for installing!' only on installing v1.0, not upgrading from 0.7.2 to this.

@westonruter westonruter self-requested a review Sep 19, 2018

@westonruter westonruter changed the title from [WIP] Display a welcome notice on the main 'AMP Settings' page to Display a welcome notice on the main 'AMP Settings' page Sep 19, 2018

Apply code review feedback for welcome message
* Refactor jQuery ready call.
* Remove debug code.
* Use regular img element for SVG as opposed to background image.
* Use curly quote instead of straight apostrophe.
* Improve event delegation.
<span class="amp-logo-icon"></span>
<h1><?php esc_html_e( 'Welcome to the AMP for WordPress plugin v1.0', 'amp' ); ?></h1>
<p><?php esc_html_e( 'Thank you for installing! Bring the speed and features of the open source AMP project to your site, the WordPress way, complete with the tools to support content authoring and website development.', 'amp' ); ?></p>
<h2><?php esc_html_e( 'What&#39;s New', 'amp' ); ?></h2>

This comment has been minimized.

@westonruter

westonruter Sep 19, 2018

Member

That's right. It should be &#8217; instead.

( function( $ ) {
$( function() {
// On dismissing the notice, make a POST request to store this notice with the dismissed WP pointers so it doesn't display again.
$( document ).on( 'click', <?php echo wp_json_encode( "#$notice_id .notice-dismiss" ); ?>, function() {

This comment has been minimized.

@westonruter

westonruter Sep 19, 2018

Member

At least we can do this:

$( <?php echo wp_json_encode( "#$notice_id" ); ?> ).on( 'click', '.notice-dismiss', function() {

That will scope the delegated event to the containing element.


<style>
.amp-logo-icon {
background-image: url( "<?php echo esc_url( amp_get_asset_url( 'images/amp-logo-icon.svg' ) ); ?>" );

This comment has been minimized.

@westonruter

westonruter Sep 19, 2018

Member

HTML entities don't render properly inside of CSS. So best to just remove esc_url() I believe.

$( function() {
// On dismissing the notice, make a POST request to store this notice with the dismissed WP pointers so it doesn't display again.
$( document ).on( 'click', <?php echo wp_json_encode( "#$notice_id .notice-dismiss" ); ?>, function() {
console.log( 'clicked' );

This comment has been minimized.

@westonruter

westonruter Sep 19, 2018

Member

Debug code to be removed.

This comment has been minimized.

@kienstra

kienstra Sep 19, 2018

Collaborator

Oh, wow. That shouldn't have been there.


<script>
( function( $ ) {
$( function() {

This comment has been minimized.

@westonruter

westonruter Sep 19, 2018

Member

This function should be combined with its parent function:

jQuery( function( $ ) {
/* ... */
} );
?>
<div class="notice notice-info is-dismissible" id="<?php echo esc_attr( $notice_id ); ?>">
<span class="amp-logo-icon"></span>

This comment has been minimized.

@westonruter

westonruter Sep 19, 2018

Member

I think this should just be a regular img.

@westonruter westonruter added this to the v1.0 milestone Sep 19, 2018

@westonruter westonruter merged commit 808a8da into develop Sep 19, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@westonruter westonruter deleted the add/1381-settings-notice branch Sep 19, 2018

@jwold

This comment has been minimized.

jwold commented Sep 20, 2018

@kienstra

I'd like to suggest the following changes to the welcome banner (note that I changed a few lines of text as well). What do you think?

illustration

And the icon:

icon.svg.zip

cc @postphotos

@jwold

This comment has been minimized.

jwold commented Sep 20, 2018

Going to actually post this to 1381, since that's the real issue :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment