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

Onboarding wizard: improve Reader mode and Transitional mode recommendations #4990

Closed
johnwatkins0 opened this issue Jul 5, 2020 · 1 comment · Fixed by #4998
Closed
Assignees
Labels
Changelogged Whether the issue/PR has been added to release notes. Enhancement New feature or improvement of an existing one Groomed WS:UX Work stream for UX/Front-end
Projects
Milestone

Comments

@johnwatkins0
Copy link
Contributor

johnwatkins0 commented Jul 5, 2020

From @westonruter in #4980 (comment).


We should export to JS this logic from PHP:

$has_amp_support = current_theme_supports( 'amp' ) || in_array( get_stylesheet(), AMP_Core_Theme_Sanitizer::get_supported_themes(), true );

Note I'm intentionally using get_stylesheet() here as opposed to get_template() because child themes of core themes are often not AMP-compatible.

Nevertheless, we may want to hold off on doing this until I finish my work on the Reader theme loading because this changes how the amp theme support is mutated (or not).

There's also a case to do something similar for Reader mode, for themes that don't have amp theme support and yet have an amp directory, which is captured by this logic at the moment:

* Gets whether the parent or child theme supports Reader Mode.
*
* True if the theme does not call add_theme_support( 'amp' ) at all,
* and it has an amp/ directory for templates.
*
* @return bool Whether the theme supports Reader Mode.
*/
public static function supports_reader_mode() {
return (
! self::get_support_mode_added_via_theme()
&&
(
is_dir( trailingslashit( get_template_directory() ) . self::READER_MODE_TEMPLATE_DIRECTORY )
||
is_dir( trailingslashit( get_stylesheet_directory() ) . self::READER_MODE_TEMPLATE_DIRECTORY )
)
);
}

We may want to mention that the active theme has special legacy Reader mode support, and also indicate that in the list of Reader themes.

@westonruter westonruter added this to the v1.6 milestone Jul 6, 2020
@westonruter westonruter added this to Backlog in Ongoing via automation Jul 6, 2020
@westonruter westonruter moved this from Backlog to To Do in Ongoing Jul 6, 2020
@kmyram kmyram added Enhancement New feature or improvement of an existing one Groomed labels Jul 7, 2020
@westonruter westonruter changed the title Onboarding wizard: improve transitional mode recommendation Onboarding wizard: improve Reader mode and Transitional mode recommendations Jul 9, 2020
@westonruter westonruter moved this from To Do to Ready for QA in Ongoing Jul 10, 2020
@pierlon
Copy link
Contributor

pierlon commented Jul 16, 2020

QA failed

Template mode recommendations are not improved and does not factor in whether the current theme is AMP compatible:

image

The AMP settings page does seem to make note of the current theme being AMP compatible, however:

image

@pierlon pierlon moved this from Ready for QA to In Progress in Ongoing Jul 16, 2020
@pierlon pierlon moved this from In Progress to To Do in Ongoing Jul 16, 2020
@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Jul 17, 2020
@westonruter westonruter moved this from To Do to QA Passed in Ongoing Jul 29, 2020
@kmyram kmyram added the WS:UX Work stream for UX/Front-end label Aug 5, 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. Enhancement New feature or improvement of an existing one Groomed WS:UX Work stream for UX/Front-end
Projects
Ongoing
  
QA Passed
Development

Successfully merging a pull request may close this issue.

4 participants