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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 2 commits into from
Dec 10, 2019
Merged

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

merged 2 commits into from
Dec 10, 2019

Conversation

scottsweb
Copy link
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 as a code owner December 9, 2019 12:23
@scottsweb scottsweb self-assigned this Dec 9, 2019
@jetpackbot
Copy link

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
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 would need you to make some changes or provide some more details about your PR. Thank you! label Dec 9, 2019
@jeherve jeherve added this to the 8.1 milestone Dec 9, 2019
@jeherve jeherve added the [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it label Dec 9, 2019
@scottsweb
Copy link
Contributor Author

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

@jeherve
Copy link
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
Contributor Author

Updated and tested

@scottsweb scottsweb added [Status] Needs Review To request a review from Crew. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Dec 9, 2019
jeherve
jeherve approved these changes Dec 9, 2019
View reviewed changes
Copy link
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 To request a review from Crew. Label will be renamed soon. 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
[Feature] Mobile Theme aka minileven [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Milestone
8.1
Development

Successfully merging this pull request may close these issues.
4 participants
@scottsweb @jetpackbot @jeherve @matticbot