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.

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

Output admin notice if persistent object caching is not present #1050

Merged

Conversation

oscarssanchez
Copy link
Contributor

@oscarssanchez oscarssanchez commented Mar 30, 2018

Hi @westonruter,

Could you use this PR which addresses #960 ?

I was not sure where it was best to write the method in, so I opted to write it in class-amp-validation-utils but i'm open to your suggestions.

I'm also not sure if the output message would be good as it is now. I opted to include a link to https://codex.wordpress.org/Class_Reference/WP_Object_Cache#Persistent_Caching since users might not know what persistent object cache is and how to enable it.

git

Fixes #960.

@oscarssanchez oscarssanchez reopened this Mar 31, 2018
@oscarssanchez oscarssanchez force-pushed the add/960-persistent-object-notice branch from ca4b925 to 59e15fb Compare March 31, 2018 02:23
@oscarssanchez
Copy link
Contributor Author

Thanks for your feedback @kienstra .

I made the corresponding changes.

@westonruter westonruter added this to the v1.0 milestone Apr 2, 2018
esc_html__( 'The AMP plugin performs at its best when persistent object cache is enabled.', 'amp' ),
esc_url( 'https://codex.wordpress.org/Class_Reference/WP_Object_Cache#Persistent_Caching' ),
esc_html__( 'More details', 'amp' ),
esc_html__( 'Dismiss this notice.', 'amp' )
Copy link
Member

Choose a reason for hiding this comment

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

I think we should eliminate the dismissal because as soon as the page reload the notice comes right back.

Copy link
Member

Choose a reason for hiding this comment

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

@oscarssanchez once this change is made the PR will be good to merge.

@oscarssanchez
Copy link
Contributor Author

oscarssanchez commented Apr 3, 2018

Hi @westonruter,

Thanks for your comments, I just did the changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants