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

Use core theme as default Reader theme if installed (as opposed to legacy) #5678

Closed
westonruter opened this issue Dec 9, 2020 · 8 comments
Closed
Labels
Groomed Maybe Later P1 Medium priority Punted Reader Mode WS:Core Work stream for Plugin core

Comments

@westonruter
Copy link
Member

Feature description

When the AMP plugin is activated and a core theme (or another AMP-compatible theme) is active, the AMP plugin defaults to Transitional mode since the theme is AMP-compatible:

// Migrate legacy method of specifying the mode.
if ( ! isset( $options[ Option::THEME_SUPPORT ] ) && $theme_support ) {
$template = get_template();
$stylesheet = get_stylesheet();
if (
// If theme support was probably explicitly added by the theme (since not core).
! in_array( $template, AMP_Core_Theme_Sanitizer::get_supported_themes(), true )
||
// If it is a core theme no child theme is being used (which likely won't be AMP-compatible by default).
$template === $stylesheet
) {
if ( empty( $theme_support[ AMP_Theme_Support::PAIRED_FLAG ] ) ) {
$defaults[ Option::THEME_SUPPORT ] = AMP_Theme_Support::STANDARD_MODE_SLUG;
} else {
$defaults[ Option::THEME_SUPPORT ] = AMP_Theme_Support::TRANSITIONAL_MODE_SLUG;
}
}
}

When the active theme is not AMP-compatible, then the plugin defaults to Reader mode, and specifically the "AMP Legacy" theme. The result is a default experience that uses our least-preferred Reader theme. We should rather check to see if a core theme is installed (as they often are) and if so, use the latest one as the default Reader theme.

So if a site has a custom theme active (which is not known to be AMP-compatible), but they also have Twenty Seventeen, Twenty Nineteen, and Twenty Twenty installed: then the default Reader theme should be Twenty Twenty since it is the most modern.


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

Implementation brief

QA testing instructions

Demo

Changelog entry

@westonruter westonruter added Reader Mode WS:Core Work stream for Plugin core labels Dec 9, 2020
@westonruter westonruter added this to the v2.1 milestone Dec 9, 2020
@westonruter westonruter changed the title Use core theme as Reader theme as default if installed Use core theme as default Reader theme if installed (as opposed to legacy) Dec 9, 2020
@westonruter
Copy link
Member Author

Related to #1384 which is about defaulting to Standard mode when the theme and plugins are all AMP-compatible. That issue was also originally about defaulting to Transitional mode when the theme is AMP-compatible, but that is now implemented.

@jwold
Copy link
Collaborator

jwold commented Jan 19, 2021

Based on our discussion:

Weston:

If a user has multiple Core themes installed it's difficult to identify the best one Core theme for them. The simplest method is to choose the most recently published. Twenty Twenty out of the box has more options out of the box. If we do always go with the most recent theme we may want a default configuration we automatically supply that normalizes the style.

We can go with reverse chronological (and offer override config), or we can go by preference, likely choosing Twenty Twenty first.

One more thing to consider, sometimes Reader Themes don't work because they have a Custom Post Type defined in the theme itself, or a recent case where a plugin was active, depended on a theme being active, and the plugin tried to call a function defined in the theme. That caused a fatal error, in that case using legacy is better.

@kmyram kmyram added Groomed P1 Medium priority labels Jan 19, 2021
@westonruter
Copy link
Member Author

If someone has a custom post type or something else defined in the active theme itself, then the legacy theme should continue to be used.

We need to do a theme compatibility check per #4795 for this as well.

@westonruter westonruter modified the milestones: v2.1, v2.2 Feb 11, 2021
@jwold
Copy link
Collaborator

jwold commented Apr 13, 2021

New theme selection in reader mode onboarding

We would like to limit the Reader theme selection screen (in the onboarding and AMP settings page) to 3 Core themes + the legacy theme.

Ordered: Twenty Twenty One, Twenty Twenty, Twenty Nineteen, AMP Legacy.

Then after those 4 we'll show installed themes.

Add documentation knowledge path

Within the theme selection screen of onboarding (and settings), add link to documentation / themes ecosystem page on amp-wp.org.

Default theme

Make Twenty Twenty One default theme.


Suggested order:

Screen Shot 2021-04-13 at 11 43 24 AM

Rough sketch of the notes above:

PNG image-94DD0AB8A17F-1

cc @amedina and @westonruter

Relates to #6036

@westonruter
Copy link
Member Author

Slight tweak to the order. I think it should be: 2021, 2020, 2019, followed by any installed core themes (or other AMP-compatible ecosystem themes) and then lastly put AMP Legacy, since it is the one we want to discourage use of the most.

The one caveat here is that Reader themes may not be available at all for the conditions outlined in #4795 (comment), but this would have already been accounted for.

@jwold
Copy link
Collaborator

jwold commented Apr 22, 2021

Action items from our 2021-04-13 Call:

  • Limit default to show 3 core themes on Vanilla WordPress. Then show the ones that are actually installed. Order by date: 2021, 2020, 2019.
  • Link to documentation on amp-wp.org for adding Reader themes; entry point for knowledge path.
  • Link to themes ecosystem page as well. May hardcode those into the plugin (not flagged auto yet).
  • Default to 2021 as Reader theme as opposed to amp legacy.

Relates to #6036

@westonruter
Copy link
Member Author

This is also somewhat related to #2313. With highlighting these core themes as Reader themes, we could eventually list other key themes from the ecosystem page that work great as Reader themes,

@westonruter
Copy link
Member Author

After thinking about this some more, I don't think we should change the default theme we activate because we need the theme scanning results in order to safely do so (#4795). Also, we want to eventually make Standard mode the default for new activations. Therefore, any change here would be just changing the order of presentation for the Reader themes. So I'm going to close this for now.

@westonruter westonruter removed this from the v2.2 milestone Jul 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Groomed Maybe Later P1 Medium priority Punted Reader Mode WS:Core Work stream for Plugin core
Projects
None yet
Development

No branches or pull requests

3 participants