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

Replace the 'Settings saved' notice, only on changing the Template Mode #1443

Merged
merged 6 commits into from Sep 22, 2018

Conversation

Projects
None yet
2 participants
@kienstra
Collaborator

kienstra commented Sep 18, 2018

  • The copy can still change, but this now has the copy here for all 3 template modes
  • Once this displays, it never displays again

Todos include:

  • At the moment, this display on the first change of any setting on this page, even 'Automatically accept sanitization...'
    It might be best to only display this message when one of the template modes changes.
  • On first activating the plugin, there probably won't be any validation errors (see #1399 (comment)). So the 'or Review errors' part of the notice might not apply. This could query for errors, and conditionally show that text.

paired-mode

Closes #1399

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

This comment has been minimized.

Member

westonruter commented Sep 18, 2018

It might be best to only display this message when one of the template modes changes.

👍

  • Once this displays, it never displays again

I don't think this should be the case necessarily. If it only shows when changing the template mode, then in reality it won't show very much.

  1. On first activating the plugin, there probably won't be any validation errors (see #1399 (comment)). So the 'or Review errors' part of the notice might not apply. This could query for errors, and conditionally show that text.

This surfaces the discussion we had awhile ago about showing the validation status alongside each of the supportable templates.

When visiting the AMP settings screen, I think we need to make sure that there are some invalid URLs captured for us to show. This could be done server-side when switching the mode, or when visiting this settings screen there could be Ajax requests triggered to validate the latest URL of each supported template type.

That may be overkill to start with, so perhaps the best thing would be upon switching the mode to obtain the first post type that is supported by AMP, and obtain the first published post of that type that has AMP enabled, and then re-validate that URL. When the page then reloads there will be validation results available to show.

if ( isset( $message ) ) {
$wp_settings_errors[ $key ]['message'] = $message; // WPCS: Global override OK.
// Ensure that this notice does not display again.
update_user_meta( get_current_user_id(), $meta_key, true );

This comment has been minimized.

@westonruter

westonruter Sep 18, 2018

Member

We can remove the user meta flag. The notice should be shown each time the user changes the mode.

This comment has been minimized.

@kienstra

kienstra Sep 18, 2018

Collaborator

Good point. 3a75250 removes the user meta as a condition of displaying this message.

}
foreach ( $wp_settings_errors as $key => $setting ) {
if ( 'general' === $setting['setting'] && 'settings_updated' === $setting['code'] ) {

This comment has been minimized.

@westonruter

westonruter Sep 18, 2018

Member

If you call add_settings_error() inside the option sanitizer callback, where we can detect the mode change, then this will get stored in a transient to then automatically be displayed with the admin_notices when the page reloads.

What this won't do is hide the default settings updated notice.

Note that add_settings_error() has a $type parameter which can be "updated", so not an error at all.

This comment has been minimized.

@kienstra

kienstra Sep 18, 2018

Collaborator

Great idea. cc2e62f uses add_settings_error() in the sanitize_callback.

Though I'm not sure why, it seems like the default message doesn't display when this is added.

on-paired-mode

Remove the user meta as a condition for displaying this message
As Weston mentioned,
it's not necessary to hide this after the first time it shows

@kienstra kienstra self-assigned this Sep 18, 2018

Only display a message if the Template Mode was changed
Thanks to Weston's suggestion,
add this in the sanitize_callback for the option.
Now, updating other options like 'Serve all templates...'
won't display this notice.

@kienstra kienstra changed the title from [WIP] Replace the 'Settings saved' notice the first time the user saves a setting to [WIP] Replace the 'Settings saved' notice, only on changing the Template Mode Sep 18, 2018

@kienstra

This comment has been minimized.

Collaborator

kienstra commented Sep 19, 2018

Validating On Switching Template Modes

Hi @westonruter,

...perhaps the best thing would be upon switching the mode to obtain the first post type that is supported by AMP, and obtain the first published post of that type that has AMP enabled, and then re-validate that URL. When the page then reloads there will be validation results available to show.

Good idea, it would make sense to validate a URL on switching template modes.

  1. Do you think this could reuse AMP_Validation_Manager:: validate_after_plugin_activation() to do this? If so, maybe we could rename it.
  2. If by chance there's no error after validating that URL, should this still have the 'or Review Errors' text, with a link to an empty Invalid URLs page? Or should this possibly query for amp_invalid_url, and only show the message with 'or Review Errors' if an error exists?
@westonruter

This comment has been minimized.

Member

westonruter commented Sep 19, 2018

@kienstra using validate_after_plugin_activation isn't quite right because it sets that transient. Otherwise, it's right. Maybe most of the guts of that method should be moved to another method, like validate_latest_post() which would store the results and and return the ID for the amp_invalid_url post.

In reality, the $validation_errors should not be stored in the transient. Only the ID should be stored. The plugin can then read the ID and then fetch the validation errors from AMP_Invalid_URL_Post_Type::get_invalid_url_validation_errors() instead of from the transient.

And yes, if there are no unaccepted validation errors on the URL then that “Review Errors” link should be removed.

@kienstra

This comment has been minimized.

Collaborator

kienstra commented Sep 20, 2018

Sorry For Delay, Will Apply Today

Hi @westonruter,
Sorry for the delay in applying your suggestions. If it's alright, I"m planning on applying them today.

@westonruter westonruter assigned westonruter and unassigned kienstra Sep 21, 2018

@westonruter

This comment has been minimized.

Member

westonruter commented Sep 21, 2018

Here's where I'm going with the messages. They vary based on the mode switched to, and there are links to view either the validated URL on the site, and/or to view the validation errors for that URL.

Success message when activating native mode:

image

Message when enabling paired mode but there are validation errors blocking it:

image

When activating classic mode:

image

@westonruter westonruter changed the title from [WIP] Replace the 'Settings saved' notice, only on changing the Template Mode to Replace the 'Settings saved' notice, only on changing the Template Mode Sep 22, 2018

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

@westonruter westonruter force-pushed the add/1399-view-as-amp branch from 2c6d27a to 75e6905 Sep 22, 2018

@westonruter westonruter merged commit ee72fa9 into develop Sep 22, 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/1399-view-as-amp branch Sep 22, 2018

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