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

Hide recommendations when Site Scan results are stale on Settings screen #6696

Merged
merged 5 commits into from
Nov 17, 2021

Conversation

delawski
Copy link
Collaborator

@delawski delawski commented Nov 8, 2021

Summary

Fixes #6690

This PR updates the Template Mode recommendation logic:

  • Remove notice about stale scan results from the Template Mode section on the AMP Settings screen.
  • Do not show any recommendations when the scan results are stale of a scan is in progress.
  • Optionally: Update recommendation descriptions/details (both in the matrix and in the code).

The screenshot below presents a case when the scan results are stale. There is no stale results notice below the Template Mode heading and no recommendation bubbles next to each template mode:

Screenshot 2021-11-08 at 13 29 07

Checklist

  • My code is tested and passes existing tests.
  • My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

@delawski delawski added this to the v2.2 milestone Nov 8, 2021
@delawski delawski requested a review from westonruter November 8, 2021 11:56
@delawski delawski force-pushed the update/template-mode-recommendations branch from 9bcddd7 to 73cd7a2 Compare November 8, 2021 12:06
@delawski delawski force-pushed the update/template-mode-recommendations branch from 73cd7a2 to 792543f Compare November 8, 2021 12:07
@delawski delawski marked this pull request as ready for review November 8, 2021 12:07
@github-actions
Copy link
Contributor

github-actions bot commented Nov 8, 2021

Plugin builds for bf16c3b are ready 🛎️!

@delawski
Copy link
Collaborator Author

delawski commented Nov 8, 2021

Optionally: Update recommendation descriptions/details (both in the matrix and in the code).

@westonruter @maitreyie-chavan I'm leaving the last action item open in case you feel like it's a good idea to update the recommendation copy right now. Other than that, this PR is ready for review.

@milindmore22
Copy link
Collaborator

This may not be related, I noticed some inconsistencies

I activate the ColorMag theme, did a site rescan It show an Incompatibility issue with colorMag theme but recommend it as Standard Mode.

image

@delawski
Copy link
Collaborator Author

@milindmore22 Thanks for testing it out.

This may not be related, I noticed some inconsistencies

It actually falls under the last action item from the description:

Optionally: Update recommendation descriptions/details (both in the matrix and in the code).

According to the current version of the recommendation matrix, if there are only theme issues and a user is technical, the Standard mode is recommended. I think the entire matrix should be revisited so that we're sure the recommendations are correct.

@dhaval-parekh
Copy link
Collaborator

Changes in PR looks good to me for the first two points. 👍

…mplate-mode-recommendations

* 'develop' of github.com:ampproject/amp-wp: (49 commits)
  Add missing translation for external link screen-reader-text
  Add vertical centering of theme screenshots
  Remove apparently obsolete .theme-browser .theme style rule
  Regenerate package-lock.json at lockfileVersion 1
  Discontinue unhooking wp_post_preview_js() in favor of dev mode
  Mark script output by wp_comment_form_unfiltered_html_nonce() as being in dev mode
  Mark wp-polyfill as a dev mode script for paired browsing
  Update Gutenberg package dependencies
  Eliminate emoji handling altogether since most browsers now support natively
  Improve copy for Sandboxing Level drawer
  Set npm to be less than v7 in engines
  Downgrade to lockfileVersion 1
  Refactor threaded comments handling to remove script if no comment form is on the page
  Update Gutenberg package dependencies
  Omit AMP runtime on sandbox levels 1 & 2 if not needed
  Ensure Sandboxing::finalize_document() is only called when experiment is enabled
  Move Sandboxing Level to Advanced Settings
  Revert package-lock.json lockfileVersion to 1
  Update amphtml spec to 2110290545003
  Add screen reader text in visit site button
  ...
@westonruter
Copy link
Member

I've updated the recommendation matrix in 817d023. The document is difficult to maintain as is the corresponding code, so I'm exclusively going off of the codebase now. I've extracted common strings into variables and then re-used them across the various scenarios.

When an AMP-incompatible theme is active:

User is technical User is non-technical
image image

For technical users, Transitional should also perhaps be NEUTRAL here.

In these screenshots, I've expanded each of the drawers to show all of the copy. It is more copy than before, for sure, but before there was not enough and I was left wondering what a template mode was. So now the same description is present for each template mode regardless of the scenario, and then prose recommendations form the next bullet point(s).

@westonruter westonruter merged commit 9069888 into develop Nov 17, 2021
@westonruter westonruter deleted the update/template-mode-recommendations branch November 17, 2021 07:02
@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Dec 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelogged Whether the issue/PR has been added to release notes. Site Scanning
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hide recommendations when Site Scan results are stale
4 participants