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

Display the "blog not public" notification where relevant #14690

Merged
merged 13 commits into from
Apr 17, 2020

Conversation

afercia
Copy link
Contributor

@afercia afercia commented Apr 1, 2020

Context

  • The site (a.k.a. "blog") not public notification is so critical that we want it to be a site wide admin notice.

Summary

This PR can be summarized in the following changelog entry:

  • Makes the "You're blocking access to robots" notification site-wide.

Relevant technical choices:

  • Uses AJAX to permanently dismiss the notice via a ignore_search_engines_discouraged_notice option
  • restores the option validation
  • deprecates blog_public_notice()
  • adds an upgrade routine to remove the previous notification from the Notification Center
  • *Important: the version used for deprecation and upgrade routine is set to 14.1

Decisions to be made:

Update: decisions were made. Please skip this part and jump tp Test Instructions.
Before I add test instructions and before CR / acceptance, there are a couple things I need feedback from product / architects related to the notice displayed in Gutenberg:

1: For product: Consider to not use HTML for the Gutenberg notice
Our notice copy is supposed to contain some strong text and a link to the Reading Settings page. A custom component for the notice text would be cool but the Gutenberg notices don't allow components to be passed for their content, which is meh 😞 The message passed to the createNotice action is casted to a string: https://github.com/WordPress/gutenberg/blob/c5fab6d23ff7478096942412bc20dc30654ca82e/packages/notices/src/store/actions.js#L57-L60

By using __unstableHTML the string can contain HTML, with two downsides:

For these reasons I'd suggest to use plain text and remove both the strong and the link.

Notice with HTML in WordPress 5.3:

notice with html

Notice with HTML in WordPress 5.4 with the Gutenberg plugin activated:

notic with html Gutenplugin

Notice with no HTML:

notice no html

1: For Software architects: Where the new code should be placed
For now, I put the new code within js/src/edit.js. This doesn't seem ideal to me for a couple reasons:

  • it would be better to import the two new functions from another file and just call it in the Edit class
  • considering that in the future we may need to add more Gutenberg Notices, I guess it would be better to have them grouped in a dedicated directory / file.

However, I couldn't think of a proper place. This isn't exactly a "component". It uses wp.data.dispatch( "core/notices" ) and related createNotice action. Any idea where to put such a thing?

Test instructions

This PR can be tested by following these steps:

  • go to WordPress > Settings > Reading Settings
  • check the "Discourage search engines from indexing this site" checkbox
  • verify the admin notice is printed out only on these pages:
    • WordPress Dashboard
    • Plugins page
    • Update core page
    • Yoast SEO settings pages
  • verify the link ''go to your Reading Settings" works
  • click the link "I don't want this site to show in the search results." and verify an AJAX request is triggered
  • check the admin notice doesn't appear any longer on all the above pages
  • optional: check in the database wp_options table > wpseo option, there's a value "ignore_search_engines_discouraged_notice";b:1;

Note: if you need to make the admin notice appear again for testing purposes, put this code somewhere in a place where it is executed: WPSEO_Options::set( 'ignore_search_engines_discouraged_notice', false );

  • don't forget to remove it 😛

UI changes

  • This PR changes the UI in the plugin. I have added the 'UI change' label to this PR.

Documentation

  • I have written documentation for this change.

Quality assurance

  • I have tested this code to the best of my abilities
  • I have added unittests to verify the code works as intended

Fixes #14566

@afercia afercia added UI change PRs that result in a change in the UI changelog: enhancement Needs to be included in the 'Enhancements' category in the changelog labels Apr 1, 2020
@afercia
Copy link
Contributor Author

afercia commented Apr 1, 2020

Regarding the admin notice outside of Gutenberg, this is how it looks like:

WP dashboard:

Screenshot 2020-04-01 at 12 10 51

@afercia
Copy link
Contributor Author

afercia commented Apr 2, 2020

Moving back to Needs Action, pending feedback.

@JessieHenkes
Copy link
Contributor

@afercia I agree with 'For these reasons I'd suggest to use plain text and remove both the strong and the link' Let's exclude HTML for the Gutenberg notice

@omarreiss
Copy link
Contributor

@afercia With #14566 (comment) your question about where the code for the notices should be placed becomes obsolete. I'm working on some big refactors for how the JavaScript in Yoast SEO is organized. I'll take this into account when doing that so it becomes more intuitive where to put that code in the future.

@afercia
Copy link
Contributor Author

afercia commented Apr 9, 2020

Thanks both for your feedback. Well at least we now have some knowledge on how to add notices to Gutenberg 🙂

Will continue to work on this tomorrow and make the notice show only where specified.

@afercia
Copy link
Contributor Author

afercia commented Apr 10, 2020

Note: the Travis build is failing for unrelated changes (JS snapshots not updated)

@afercia
Copy link
Contributor Author

afercia commented Apr 14, 2020

  • renamed methods and option based on the phrasing "search engines discouraged"
  • updated test instructions accordingly

@afercia afercia changed the title Make the "blog not public" notification site wide Display the "blog not public" notification where relevant Apr 14, 2020
@karlijnbok
Copy link
Contributor

CR & Acceptance: 👍

@karlijnbok karlijnbok added this to the 14.1 milestone Apr 17, 2020
@karlijnbok karlijnbok merged commit 4c386c9 into trunk Apr 17, 2020
@karlijnbok karlijnbok deleted the 14566-blog-public-notification-site-wide branch April 17, 2020 07:18
@afercia
Copy link
Contributor Author

afercia commented Apr 21, 2020

Note: I think the removal of the old notification in the Notification Center is suffering from the same problem described on #14828 (comment)

Will re-test and fix if need be.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: enhancement Needs to be included in the 'Enhancements' category in the changelog UI change PRs that result in a change in the UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make wpseo-dismiss-public-notice a site wide notification
4 participants