Skip to content

Allow mobile theme to be removed via query string for testing#14196

Merged
scottsweb merged 2 commits intomasterfrom
add/test-query-string-deprecation-minileven
Dec 10, 2019
Merged

Allow mobile theme to be removed via query string for testing#14196
scottsweb merged 2 commits intomasterfrom
add/test-query-string-deprecation-minileven

Conversation

@scottsweb
Copy link
Copy Markdown
Contributor

@scottsweb scottsweb commented Dec 9, 2019 •
edited
Loading

Testing an idea to allow temporary removal of minileven for testing within a responsive website test tool.

Changes proposed in this Pull Request:

  • Allow mobile theme to be removed via query string for testing

Is this a new feature or does it add/remove features to an existing part of Jetpack?

Modify existing part of Jetpack: p6TEKc-3iM-p2

Testing instructions:

  • Enable the mobile theme
  • Test the mobile theme works
  • Add ?jetpack-preview=responsivetheme to the URL and test the original theme loads
  • Remove or modify query string to ensure mobile theme still works

Proposed changelog entry for your changes:

  • N/A

@scottsweb
[not verified]
0f0c348 
Allow mobile theme to be removed via query string for testing
@scottsweb scottsweb requested a review from a team December 9, 2019 12:23
@scottsweb scottsweb self-assigned this Dec 9, 2019
@jetpackbot
Copy link
Copy Markdown
Collaborator

jetpackbot commented Dec 9, 2019 •
edited
Loading

Warnings
⚠️

pre-commit hook was skipped for one or more commits

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against c17c6e8

jeherve
jeherve requested changes Dec 9, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

What would you think about moving this here instead?

jetpack/modules/minileven/minileven.php

Line 31 in 792b26b

if ( ( defined('XMLRPC_REQUEST') && XMLRPC_REQUEST ) || ( defined('APP_REQUEST') && APP_REQUEST ) )

@jeherve jeherve added the [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. label Dec 9, 2019
@jeherve jeherve added this to the 8.1 milestone Dec 9, 2019
@jeherve jeherve added the Enhancement Changes to an existing feature — removing, adding, or changing parts of it label Dec 9, 2019
@scottsweb
Copy link
Copy Markdown
Contributor Author

scottsweb commented Dec 9, 2019

That looks like it could be a more sensible place to check, I will do some testing and make the change.

@jeherve
Copy link
Copy Markdown
Member

jeherve commented Dec 9, 2019

Maybe we can take that opportunity to give the query string a more generic name? jptest may seem a bit odd to folks seeing it in their logs.

Something like jetpack-preview may be more descriptive?

@scottsweb
Copy link
Copy Markdown
Contributor Author

scottsweb commented Dec 9, 2019

Updated and tested

@scottsweb scottsweb added [Status] Needs Review This PR is ready for review. and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Dec 9, 2019
jeherve
jeherve approved these changes Dec 9, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

LGTM! Let's get this in!

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Dec 9, 2019
@scottsweb scottsweb merged commit 8c90318 into master Dec 10, 2019
@scottsweb scottsweb deleted the add/test-query-string-deprecation-minileven branch December 10, 2019 10:26
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Dec 10, 2019
jeherve added a commit that referenced this pull request Dec 13, 2019
@jeherve
Changelog: add #14196
988fac2
jeherve added a commit to Automattic/wp-calypso that referenced this pull request Dec 17, 2019
@jeherve
Customize Mobile Theme card depending on version and module status
caf1825 
- If site runs Jetpack 8.1-alpha or newer, we know they have access to the new query string introduced here: Automattic/jetpack#14196 We can consequently show them a notice with a link to check if site is responsive at Google.
- If site runs an older version of Jetpack, or if the Mobile Theme is disabled, show a regular deprecation notice.
- Do not display mobile theme toggle or settings when module is deactivated.
jeherve added a commit to Automattic/wp-calypso that referenced this pull request Dec 18, 2019
@jeherve
Writing Settings: disable Mobile Theme UI when feature isn't a… (#37857)
2937291 
* Writing Settings: remove Mobile Theme UI when feature isn't active

Fixes #36404

* Customize Mobile Theme card depending on version and module status

- If site runs Jetpack 8.1-alpha or newer, we know they have access to the new query string introduced here: Automattic/jetpack#14196 We can consequently show them a notice with a link to check if site is responsive at Google.
- If site runs an older version of Jetpack, or if the Mobile Theme is disabled, show a regular deprecation notice.
- Do not display mobile theme toggle or settings when module is deactivated.

* Fix form class name

It did not reflect the actual section we are in.

* Grey out settings instead of removing them when module inactive

* Remove unneeded fragment
zinigor added a commit that referenced this pull request Dec 30, 2019
@jeherve @zinigor
Changelog: 8.1 additions (#14166)
fff089c 
* Changelog: 8.1 additions

* Changelog: add #13858

* Changelog: add #13963

* Changelog: add #14174

* Changelog: add #14178

* Changelog: add #14175

* Changelog: add #14192

* Changelog: add #14196

* Changelog: add #14182

* Changelog: add #14218

* Changelog: add #14214

* Changelog: add #13757

* Changelog: add #14190

* Changelog: add #14131

* Changelog: add #14101

* Changelog: add #14203

* Changelog: add #14211

* Changelog: add #14224

* Changelog: add #14230

* Changelog: add #14241

* Changelog: add #14249

* Changelog: add #14264

* Changelog: add #14263

* Changelog: add #14256

* Changelog: add #10189

* Changelog: add #14240

* Changelog: add #14239

Also added some new entries to the testing file.

Co-authored-by: Igor Zinovyev <zinigor@gmail.com>
zinigor added a commit that referenced this pull request Dec 30, 2019
@jeherve @zinigor
Changelog: 8.1 additions (#14166)
d4a7565 
* Changelog: 8.1 additions

* Changelog: add #13858

* Changelog: add #13963

* Changelog: add #14174

* Changelog: add #14178

* Changelog: add #14175

* Changelog: add #14192

* Changelog: add #14196

* Changelog: add #14182

* Changelog: add #14218

* Changelog: add #14214

* Changelog: add #13757

* Changelog: add #14190

* Changelog: add #14131

* Changelog: add #14101

* Changelog: add #14203

* Changelog: add #14211

* Changelog: add #14224

* Changelog: add #14230

* Changelog: add #14241

* Changelog: add #14249

* Changelog: add #14264

* Changelog: add #14263

* Changelog: add #14256

* Changelog: add #10189

* Changelog: add #14240

* Changelog: add #14239

Also added some new entries to the testing file.

Co-authored-by: Igor Zinovyev <zinigor@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jeherve jeherve jeherve approved these changes

Assignees

@scottsweb scottsweb

Labels

Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Feature] Mobile Theme aka minileven

Projects

None yet

Milestone

8.1

Development

Successfully merging this pull request may close these issues.

4 participants

@scottsweb @jetpackbot @jeherve @matticbot