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

Fallback to AMP Legacy Reader theme if active Reader theme is unavailable #5159

Merged
merged 38 commits into from
Aug 8, 2020

Conversation

pierlon
Copy link
Contributor

@pierlon pierlon commented Aug 4, 2020

Summary

  • Obtains current Reader theme data from list of installed WP themes if the supplied Reader theme data becomes unavailable.
  • Adds default mobile and desktop image placeholder for themes without a screenshot.
    • Fixes how the desktop screenshot is shown when displayed in the mobile phone container.
  • Falls back to the legacy Reader theme if the current Reader theme becomes unavailable.
    • Shows an admin warning notice when in this scenario.

Screenshots:

  • Desktop and mobile image placeholders

image

  • Admin warning notice when the AMP legacy theme is used as a fallback, on the Themes and AMP Settings screens:

image

image

Fixes #5070
Fixes #5136

Checklist

  • My pull request is addressing an open issue (please create one otherwise).
  • 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).

@google-cla google-cla bot added the cla: yes Signed the Google CLA label Aug 4, 2020
@pierlon pierlon force-pushed the fix/5070-custom-reader-theme-info branch from e50a6a2 to aeeb22e Compare August 7, 2020 00:47
@pierlon pierlon marked this pull request as ready for review August 7, 2020 05:08
@github-actions
Copy link
Contributor

github-actions bot commented Aug 7, 2020

Plugin builds for 810ec5f are ready 🛎️!

@westonruter westonruter added this to the v2.0 milestone Aug 7, 2020
@westonruter
Copy link
Member

Investigating the test failures…

@pierlon
Copy link
Contributor Author

pierlon commented Aug 7, 2020

Already fixed the E2E ones; pushing fix now.

src/Admin/OnboardingWizardSubmenu.php Outdated Show resolved Hide resolved
src/Admin/OnboardingWizardSubmenu.php Outdated Show resolved Hide resolved
src/Admin/ReaderThemes.php Show resolved Hide resolved
tests/php/test-amp-gallery-embed-handler.php Outdated Show resolved Hide resolved
@westonruter
Copy link
Member

@pierlon There's a condition here that may not be accounted for. If I have a plugin present that adding Neve via amp_reader_themes filter, and I select Neve as the Reader theme, but then I delete neve theme, I get an unexpected state where Neve is still the selected theme even though it is not installed:

image

The only way for me to proceed I believe is to select a non-Neve theme and then re-select Neve, which isn't expected. In the case of Neve still being among the selectable Reader themes but not being installed anymore, the Legacy theme should be selected in that case as well, I suppose?

@pierlon
Copy link
Contributor Author

pierlon commented Aug 7, 2020

Good catch.

In the case of Neve still being among the selectable Reader themes but not being installed anymore, the Legacy theme should be selected in that case as well, I suppose?

Yep that's right. Currently it's only checking if the theme data exists, but it should be instead checking if the theme exists.

@pierlon
Copy link
Contributor Author

pierlon commented Aug 7, 2020

@westonruter e5e086f should fix the issue.

@pierlon pierlon force-pushed the fix/5070-custom-reader-theme-info branch from ef95ca8 to 9364a51 Compare August 7, 2020 23:53
@pierlon pierlon force-pushed the fix/5070-custom-reader-theme-info branch from 9364a51 to 3f005bf Compare August 7, 2020 23:54
Comment on lines 1758 to 1759
delete_site_transient( 'theme_roots' );

Copy link
Member

@westonruter westonruter Aug 8, 2020

Choose a reason for hiding this comment

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

It doesn't appear that this does anything anymore:

Suggested change
delete_site_transient( 'theme_roots' );

Copy link
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

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

🎉

@@ -18,6 +18,9 @@ module.exports = {
'<rootDir>/build/',
'<rootDir>/tests/shared',
],
modulePathIgnorePatterns: [
'<rootDir>/assets/src/components/.*/__mocks__',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I received the following warning while running the JS tests:

jest-haste-map: duplicate manual mock found: index
  The following files share their name; please delete one of them:
    * <rootDir>/assets/src/components/options-context-provider/__mocks__/index.js
    * <rootDir>/assets/src/components/reader-themes-context-provider/__mocks__/index.js

It seems Jest doesn't allow mocks with the same base name for some weird reason. It's been filed as a bug for quite some time now.

Ignoring the mocks in assets/src/components seems to resolve the issue, but I'd like to get a second opinion from @johnwatkins0 on this.

@westonruter westonruter merged commit a8916b8 into develop Aug 8, 2020
@westonruter westonruter deleted the fix/5070-custom-reader-theme-info branch August 8, 2020 02:24
@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Aug 8, 2020
@johnwatkins0 johnwatkins0 added the WS:UX Work stream for UX/Front-end label Aug 12, 2020
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. cla: yes Signed the Google CLA WS:UX Work stream for UX/Front-end
Projects
None yet
3 participants