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

Prevent non-legacy themes from being selected as Reader theme on Settings screen when AMP slug defined late #5789

Closed
westonruter opened this issue Jan 19, 2021 · 1 comment · Fixed by #5859
Assignees
Labels
Bug Something isn't working Groomed P1 Medium priority WS:UX Work stream for UX/Front-end
Projects
Milestone

Comments

@westonruter
Copy link
Member

Bug Description

In #4999 we check if the AMP slug (query var) is defined too late and if so, prevent selection of a non-legacy Reader theme:

image

This, however, was not likewise implemented for the Settings screen which allows all to be selected:

image

Expected Behaviour

The available themes should be consistent between the Settings screen and the Onboarding Wizard.

Steps to reproduce

Activate this plugin:

<?php
/**
 * Plugin Name: AMP Define Query Var Early with Constant
 */

add_action(
	'after_setup_theme',
	function () {
		add_filter(
			'amp_query_var',
			function() {
				return 'too-early-with-constant';
			}
		);
	},
	~PHP_INT_MAX
);

Then access the AMP Settings screen and select Reader template mode.

This is part of #4795.


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 Bug Something isn't working WS:UX Work stream for UX/Front-end labels Jan 19, 2021
@westonruter westonruter added this to the v2.1 milestone Jan 19, 2021
@westonruter westonruter added this to Backlog in Ongoing via automation Jan 19, 2021
@westonruter westonruter moved this from Backlog to To Do in Ongoing Jan 19, 2021
@kmyram kmyram added P1 Medium priority Groomed labels Feb 2, 2021
@delawski delawski moved this from To Do to In Progress in Ongoing Feb 4, 2021
@delawski delawski moved this from In Progress to Ready for Review in Ongoing Feb 5, 2021
@westonruter westonruter moved this from Ready for Review to Ready for QA in Ongoing Apr 22, 2021
@westonruter westonruter assigned westonruter and unassigned delawski Apr 22, 2021
@westonruter
Copy link
Member Author

QA Passed

To test this, I added a plugin that did this:

add_action( 'setup_theme', function () {
	define( 'AMP_QUERY_VAR', 'lite' );
} );

When doing so, the settings screen shows:

image

And the Onboarding Wizard:

image

So the non-legacy Reader themes are indeed prevented from being used.

@westonruter westonruter moved this from Ready for QA to QA Passed in Ongoing Apr 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Groomed P1 Medium priority WS:UX Work stream for UX/Front-end
Projects
Ongoing
  
QA Passed
3 participants