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
Merged
Show file tree
Hide file tree
Changes from 36 commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
fdedae8
Show mobile icon placeholder if none was defined
pierlon Jul 31, 2020
3c9b293
Show desktop icon placeholder if none was defined
pierlon Aug 3, 2020
fd55eb7
Obtain active reader theme data from list of installed themes if avai…
pierlon Aug 4, 2020
8b6b1e9
Fallback to legacy Reader mode if active Reader theme is unavailable
pierlon Aug 4, 2020
289dce4
Fix lint errors
pierlon Aug 4, 2020
3d39560
Revert making Reader theme service conditional
pierlon Aug 6, 2020
aeeb22e
Fix how legacy mode is determined
pierlon Aug 6, 2020
030df93
Merge branch 'develop' into fix/5070-custom-reader-theme-info
pierlon Aug 7, 2020
688b8b2
Center desktop screenshots when viewed in mobile viewport
pierlon Aug 7, 2020
b76fe2d
Align SVG content to canvas
pierlon Aug 7, 2020
a9cba2b
Show an admin notice on Settings page if Reader theme becomes unavail…
pierlon Aug 7, 2020
bf6ca59
Add test to verify Reader theme data can be retrieved from list of in…
pierlon Aug 7, 2020
3575517
Show mobile image placeholder in onboarding wizard
pierlon Aug 7, 2020
5a71cf6
Use already existing reader theme
pierlon Aug 7, 2020
6c34896
Fix test-amp-helper-functions for core themes in src
westonruter Aug 7, 2020
05a673d
Simplify logic in amp_is_legacy()
westonruter Aug 7, 2020
d03eaf9
Let child-of-core be child of Twenty Seventeen to fix WP<5.3 tests
westonruter Aug 7, 2020
e8e1d93
Fix tests in WP 4.9 and ensure core themes in src are registered
westonruter Aug 7, 2020
b2f4ed0
Programmatically select the AMP Legacy theme if it's used as a fallback
pierlon Aug 7, 2020
f3707a1
Make registration of core theme directory in tests DRY
pierlon Aug 7, 2020
1d939c4
Add link to reader themes drawer
westonruter Aug 7, 2020
17261a3
Restrict showing legacy fallback notice to admins on themes screen an…
westonruter Aug 7, 2020
6bb44d7
Only programmmatically select legacy theme when current Reader theme …
pierlon Aug 7, 2020
e5e086f
Ensure AMP legacy theme is being used as a fallback
pierlon Aug 7, 2020
c947bc9
Remove user capability check from ReaderThemes::using_fallback_theme
pierlon Aug 7, 2020
b2a9716
Replace astra with neve for testing since astra is suspended
westonruter Aug 7, 2020
0335d92
Enable the "Customize Theme" button after installing the new Reader t…
pierlon Aug 7, 2020
b322c37
Remove screenshot from being required for DesktopScreenshot.propTypes
westonruter Aug 7, 2020
3f005bf
Remove outdated hook dependencies
pierlon Aug 7, 2020
88fa245
Add tests for admin notice printers
westonruter Aug 8, 2020
5f65e7d
Add test for ReaderThemes::using_fallback_theme
westonruter Aug 8, 2020
7029023
Remove unused asset dependency
pierlon Aug 8, 2020
cda8156
Explicitly ignore legacy theme since not yet pushed to array
westonruter Aug 8, 2020
06ac5ed
Make ReaderThemes::normalize_theme_data() private
westonruter Aug 8, 2020
83cb2c5
Reuse variable for condition
westonruter Aug 8, 2020
877b4ab
Use LoadsCoreThemes in remaining tests
westonruter Aug 8, 2020
f2df5c7
Delete obsolete deletion of theme_roots site transient
westonruter Aug 8, 2020
810ec5f
Mock Reader themes provider
pierlon Aug 8, 2020
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion assets/src/components/phone/style.css
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
display: flex;
flex-direction: column;
margin-bottom: 1rem;
max-height: 413px;
height: 413px;
min-height: 200px;
padding: 12px;
position: relative;
Expand All @@ -25,5 +25,8 @@
}

.phone-inner {
display: flex;
background-color: var(--color-gray-medium);
flex-grow: 1;
overflow: hidden;
}
13 changes: 9 additions & 4 deletions assets/src/components/reader-themes-context-provider/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import { __ } from '@wordpress/i18n';
* External dependencies
*/
import PropTypes from 'prop-types';
import { USING_FALLBACK_READER_THEME, LEGACY_THEME_SLUG } from 'amp-settings';

/**
* Internal dependencies
Expand Down Expand Up @@ -80,16 +81,20 @@ export function ReaderThemesContextProvider( { wpAjaxUrl, children, currentTheme
const [ downloadingThemeError, setDownloadingThemeError ] = useState( null );

/**
* If the currently selected theme is unavailable and not installable, or the current theme is the active theme,
* unset the reader theme option.
* If the currently selected theme is not installable, is the active theme, or unavailable for selection, set the
* Reader theme to AMP Legacy.
*/
useEffect( () => {
if ( themeWasOverridden ) { // Only do this once.
return;
}

if ( selectedTheme.availability === 'non-installable' || originalSelectedTheme.availability === 'active' ) {
updateOptions( { reader_theme: 'legacy' } );
if (
selectedTheme.availability === 'non-installable' ||
originalSelectedTheme.availability === 'active' ||
USING_FALLBACK_READER_THEME
) {
updateOptions( { reader_theme: LEGACY_THEME_SLUG } );
setThemeWasOverridden( true );
}
}, [ originalSelectedTheme.availability, selectedTheme.availability, themeWasOverridden, updateOptions ] );
Expand Down
49 changes: 49 additions & 0 deletions assets/src/components/svg/desktop-icon.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
42 changes: 42 additions & 0 deletions assets/src/components/svg/mobile-icon.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
25 changes: 15 additions & 10 deletions assets/src/components/theme-card/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,11 @@ import PropTypes from 'prop-types';
/**
* Internal dependencies
*/
import './style.css';
import MobileIcon from '../svg/mobile-icon.svg';
import { Options } from '../options-context-provider';
import { Selectable } from '../selectable';
import { Phone } from '../phone';
import './style.css';

/**
* A selectable card showing a theme in a list of themes.
Expand Down Expand Up @@ -48,14 +49,18 @@ export function ThemeCard( { description, ElementName = 'li', homepage, screensh
>
<label htmlFor={ id } className="theme-card__label">
<Phone>
<img
src={ screenshotUrl }
alt={ name }
height="2165"
width="1000"
loading="lazy"
decoding="async"
/>
{
screenshotUrl ? (
<img
src={ screenshotUrl }
alt={ name }
height="2165"
width="1000"
loading="lazy"
decoding="async"
/>
) : <MobileIcon style={ { width: '100%' } } />
}
{ disabled && (
<div className="theme-card__disabled-overlay">
{ __( 'Unavailable', 'amp' ) }
Expand Down Expand Up @@ -96,7 +101,7 @@ ThemeCard.propTypes = {
description: PropTypes.string.isRequired,
ElementName: PropTypes.string,
homepage: PropTypes.string.isRequired,
screenshotUrl: PropTypes.string.isRequired,
screenshotUrl: PropTypes.string,
slug: PropTypes.string.isRequired,
name: PropTypes.string.isRequired,
disabled: PropTypes.bool,
Expand Down
5 changes: 1 addition & 4 deletions assets/src/components/theme-card/style.css
Original file line number Diff line number Diff line change
Expand Up @@ -48,13 +48,10 @@
}
}

.theme-card .phone-inner {
max-height: 307px; /* Make all images uniform height. */
}

.theme-card img {
height: auto;
width: 100%;
margin: auto;
}

p.theme-card__description {
Expand Down
4 changes: 3 additions & 1 deletion assets/src/onboarding-wizard/components/nav/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import './style.css';
import { Options } from '../../../components/options-context-provider';
import { User } from '../user-context-provider';
import { READER } from '../../../common/constants';
import { ReaderThemes } from '../../../components/reader-themes-context-provider';

/**
* Navigation component.
Expand All @@ -36,6 +37,7 @@ export function Nav( { closeLink, finishLink } ) {
originalOptions: { preview_permalink: previewPermalink, reader_theme: readerTheme },
} = useContext( Options );
const { savingDeveloperToolsOption } = useContext( User );
const { downloadingTheme } = useContext( ReaderThemes );

let nextText;
let nextLink;
Expand Down Expand Up @@ -97,7 +99,7 @@ export function Nav( { closeLink, finishLink } ) {
}

<Button
disabled={ ! canGoForward || savingOptions || savingDeveloperToolsOption }
disabled={ ! canGoForward || savingOptions || savingDeveloperToolsOption || downloadingTheme }
href={ nextLink }
id="next-button"
isPrimary
Expand Down
23 changes: 14 additions & 9 deletions assets/src/onboarding-wizard/pages/summary/desktop-screenshot.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,21 +12,26 @@ import { __ } from '@wordpress/i18n';
* Internal dependencies
*/
import { Desktop } from '../../components/desktop';
import DesktopIcon from '../../../components/svg/desktop-icon.svg';

export function DesktopScreenshot( { screenshot, name, description, url } ) {
return (
<div className="selectable selectable--bottom">

<div className="grid grid-1-2 summary-screenshot">
<Desktop>
<img
src={ screenshot }
alt={ name }
loading="lazy"
decoding="async"
height="900"
width="1200"
/>
{
screenshot ? (
<img
src={ screenshot }
alt={ name }
loading="lazy"
decoding="async"
height="900"
width="1200"
/>
) : <DesktopIcon />
}
</Desktop>
<div>
<h3>
Expand All @@ -50,6 +55,6 @@ export function DesktopScreenshot( { screenshot, name, description, url } ) {
DesktopScreenshot.propTypes = {
description: PropTypes.string.isRequired,
name: PropTypes.string.isRequired,
screenshot: PropTypes.string.isRequired,
screenshot: PropTypes.string,
url: PropTypes.string.isRequired,
};
42 changes: 26 additions & 16 deletions assets/src/onboarding-wizard/pages/summary/reader.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ import { IconMobile } from '../../../components/svg/icon-mobile';
import { Options } from '../../../components/options-context-provider';
import { RedirectToggle } from '../../../components/redirect-toggle';
import { ReaderThemes } from '../../../components/reader-themes-context-provider';
import DesktopIcon from '../../../components/svg/desktop-icon.svg';
import MobileIcon from '../../../components/svg/mobile-icon.svg';
import { SummaryHeader } from './summary-header';

/**
Expand Down Expand Up @@ -57,14 +59,18 @@ export function Reader( { currentTheme } ) {
</AMPInfo>

<Desktop>
<img
src={ currentTheme.screenshot }
alt={ currentTheme.name }
loading="lazy"
decoding="async"
height="900"
width="1200"
/>
{
currentTheme.screenshot ? (
<img
src={ currentTheme.screenshot }
alt={ currentTheme.name }
loading="lazy"
decoding="async"
height="900"
width="1200"
/>
) : <DesktopIcon />
}

</Desktop>

Expand All @@ -89,14 +95,18 @@ export function Reader( { currentTheme } ) {
</AMPInfo>

<Phone>
<img
src={ readerThemeData.screenshot_url }
alt={ readerThemeData.name }
loading="lazy"
decoding="async"
height="2165"
width="1000"
/>
{
readerThemeData.screenshot_url ? (
<img
src={ readerThemeData.screenshot_url }
alt={ readerThemeData.name }
loading="lazy"
decoding="async"
height="2165"
width="1000"
/>
) : <MobileIcon style={ { width: '100%' } } />
}
</Phone>

<h3>
Expand Down
1 change: 1 addition & 0 deletions assets/src/onboarding-wizard/pages/summary/style.css
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
.reader-summary__screens img {
width: 100%;
height: auto;
margin: auto;
}

.reader-summary__screens .selectable {
Expand Down
2 changes: 1 addition & 1 deletion assets/src/onboarding-wizard/style.css
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ body {
position: relative;

@media screen and (min-width: 783px) {
padding: 15px 21px 8px 0;
padding: 15px 10px 8px 0;
}
}

Expand Down
4 changes: 0 additions & 4 deletions assets/src/settings-page/style.css
Original file line number Diff line number Diff line change
Expand Up @@ -59,10 +59,6 @@
}
}

.amp-settings .theme-card .phone-inner {
max-height: 298px;
}

/* Supported templates section. */
.supported-templates {
margin-bottom: 3rem;
Expand Down
15 changes: 10 additions & 5 deletions includes/amp-helper-functions.php
Original file line number Diff line number Diff line change
Expand Up @@ -364,11 +364,16 @@ function amp_is_canonical() {
* @return bool
*/
function amp_is_legacy() {
return (
AMP_Theme_Support::READER_MODE_SLUG === AMP_Options_Manager::get_option( Option::THEME_SUPPORT )
&&
ReaderThemes::DEFAULT_READER_THEME === AMP_Options_Manager::get_option( Option::READER_THEME )
);
if ( AMP_Theme_Support::READER_MODE_SLUG !== AMP_Options_Manager::get_option( Option::THEME_SUPPORT ) ) {
return false;
}

$reader_theme = AMP_Options_Manager::get_option( Option::READER_THEME );
if ( ReaderThemes::DEFAULT_READER_THEME === $reader_theme ) {
return true;
}

return ! wp_get_theme( $reader_theme )->exists();
}

/**
Expand Down
Loading