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

Mobile Theme: remove feature #14714

Merged
merged 9 commits into from
Feb 18, 2020
Merged

Mobile Theme: remove feature #14714

merged 9 commits into from
Feb 18, 2020

Conversation

jeherve
Copy link
Member

@jeherve jeherve commented Feb 18, 2020 •
edited by kraftbj
Loading

Fixes #13385

Closes #6616
Closes #5815
Closes #6617
Closes #10685
Closes #2061

Changes proposed in this Pull Request:

It is time.

We originally created the mobile theme feature as a fall-back when the regular theme did not include a mobile view. Most themes include a mobile view by default now, so the feature is no longer necessary.

This PR is the last step in deprecating the Mobile Theme feature. In #14101 (as well as messages in the dashboard and emails), we started warning folks that the feature was going away. In this PR, we remove the feature entirely:

  • We deactivate the module if it still active.
  • We deactivate the module on plugin update.
  • We delete options created by the module to clean things up.
  • We remove the UI from the Jetpack Settings dashboard.
  • We output notices to folks calling functions that were used by the module, or calling the module file directly.
  • We stop synchronizing options with WordPress.com.
  • We output notices to all folks who may still use filters / hooks to customize the mobile theme.

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

  • It removes an existing feature. Internal reference: p1HpG7-85q-p2

Testing instructions:

  • Start from a site that does not run this branch.
  • Run wp jetpack module activate minileven to activate the Mobile Theme.
  • Switch to this branch.
  • The Mobile Theme should not be active when you visit your site on a mobile device.
  • You should see no PHP errors.

Proposed changelog entry for your changes:

  • Mobile Theme: sunset feature. We originally created the mobile theme feature as a fall-back when the regular theme did not include a mobile view. Most themes include a mobile view by default now, so the feature is no longer necessary.

@jeherve
Remove Minileven UI from Jetpack Settings dashboard
ad1ef46
@jeherve
Remove PHP settings for Minileven
1e36019 
We don't need a module description nor options for the module anymore.
@jeherve
Do not synchronize minileven options with WordPress.com anynore
65ae4a4
@jeherve
Remove Minileven's theme files
d56610d
@jeherve
Remove conflicting plugin for now-sunset minileven module
1ec3c11
@jeherve
Delete minileven module options on plugin update
5d547ed 
We don't need them anymore, let's clean up behind ourselves
@jeherve
Add minileven to list of deprecated modules
a28aad9
@jeherve
Mark all hooks used in module as deprecated with no replacement
439db51
@jeherve
Deprecate Mobile Theme.
a11eeb0 
- Add comment explaining the reasons behind sunsetting.
- Deactivate the module if it is still active.
- Output error when module file is called directly.
- Mark functions that may be used by external plugins as deprecated.
@jeherve jeherve added [Feature] Mobile Theme aka minileven [Status] Needs Review To request a review from Crew. Label will be renamed soon. [Type] Janitorial [Pri] Normal labels Feb 18, 2020
@jeherve jeherve added this to the 8.3 milestone Feb 18, 2020
@jeherve jeherve requested review from a team as code owners February 18, 2020 12:09
@jeherve jeherve self-assigned this Feb 18, 2020
jeherve
jeherve commented Feb 18, 2020
View reviewed changes
class.jetpack-modules-list-table.php
@@ -96,9 +96,7 @@ function js_templates() {
<# var i = 0;
if ( data.items.length ) {
_.each( data.items, function( item, key, list ) {
if ( item === undefined ) return;
if ( 'minileven' == item.module && ! item.activated ) return;
if ( 'manage' == item.module && item.activated ) return; #>
Copy link
Member Author

Choose a reason for hiding this comment

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

I took the opportunity to delete this reference to the Manage module.

@jetpackbot
Copy link

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: March 3, 2020.
Scheduled code freeze: February 25, 2020

Generated by 🚫 dangerJS against a11eeb0

@kraftbj kraftbj self-requested a review February 18, 2020 14:19
@kraftbj
Copy link
Contributor

kraftbj commented Feb 18, 2020

Reads well. Going to spin up a JN to poke around a bit.

kraftbj
kraftbj approved these changes Feb 18, 2020
View reviewed changes
Copy link
Contributor

@kraftbj kraftbj left a comment

Choose a reason for hiding this comment

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

Noting that this notice in Calypso needs to be removed sooner rather than later:

2020-02-18 at 8 54 AM

But functionally, it doesn't allow me to activate the mobile theme, so that's not a blocker for the PR.

Otherwise, I see no mention of the mobile theme nor errors. 🚢

@kraftbj kraftbj 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 Feb 18, 2020
@jeherve
Copy link
Member Author

jeherve commented Feb 18, 2020

Noting that this notice in Calypso needs to be removed sooner rather than later

Yup, that's next on my list, and will be gone for anyone running Jetpack 8.3+.

@jeherve jeherve merged commit 6bc9e35 into master Feb 18, 2020
@jeherve jeherve deleted the rm/minileven-module branch February 18, 2020 15:31
@matticbot matticbot added [Status] Needs Changelog [Status] Needs Package Release This PR made changes to a package. Let's update that package now. labels Feb 18, 2020
@matticbot matticbot removed the [Status] Ready to Merge Go ahead, you can push that green button! label Feb 18, 2020
@jeherve jeherve mentioned this pull request Feb 18, 2020
Merged
@jeherve jeherve mentioned this pull request Feb 21, 2020
Merged
jeherve added a commit that referenced this pull request Feb 21, 2020
@jeherve
Minileven: add options back as they still exist on sites
523f4ed 
This is a follow-up to #14714.
After removing those options, we started seeing errors like this one on sites:
PHP Warning: Invalid Jetpack option name: wp_mobile_excerpt

Since these Jetpack options are still valid options even if the feature is gone, let's keep them around.
jeherve added a commit that referenced this pull request Feb 25, 2020
@jeherve
Minileven: add options back as they still exist on sites (#14756)
23cbea4 
* Minileven: add options back  as they still exist on sites

This is a follow-up to #14714.
After removing those options, we started seeing errors like this one on sites:
PHP Warning: Invalid Jetpack option name: wp_mobile_excerpt

Since these Jetpack options are still valid options even if the feature is gone, let's keep them around.

* Those options are not stored as Jetpack options.

See #14756 (comment)

* Remove unneeded options

See https://github.com/Automattic/jetpack/pull/14756/files#r383413182

* Only delete options if the standalone plugin is not in use
jeherve added a commit that referenced this pull request Feb 25, 2020
@jeherve
Changelog: add #14714
94869af
@jeherve jeherve removed the [Status] Needs Package Release This PR made changes to a package. Let's update that package now. label Feb 25, 2020
jeherve added a commit that referenced this pull request Feb 25, 2020
@jeherve
Changelog: 8.3 running changelog (#14642)
8797bea 
* 8.3 release: changelog

* Changelog: add #14516

* Changelog: add #14574

* Bring in changes from 8.2.1 and 8.2.2

* Update stable version

* Bring in 8.2.3 changes

* Changelog: add #14714

* Changelog: add #14639

* Changelog: add #14678

* Changelog: add #14673

* Changelog: add #14687

* Changelog: add #14704

* Changelog: add #14702

* Changelog: add #14541

* Changelog: add #14657

* Changelog: add #14622

* Changelog: add #14582

* Changelog: add #14638

* Changelog: add #14633

* Changelog: add #14571

* Changelog: add #14592

* Changelog: add #14539

* Changelog: add #14514

* Changelog: add #14643

* Changelog: add #14494

* Changelog: add #13739

* Changelog: add #14707

* Changelog: add #14736

* Changelog: add #14706

* Changelog: add #14730

* Changelog: add #14685

* Changelog: add #14727

* Changelog: add #14711

* Changelog: add #14742

* Changelog: add #14746

* Changelog: add #14725

* Changelog: add #13999

* Changelog: add #14740

* Changelog: add #14759

* Changelog: add #14703

* Changelog: add #14753

* Changelog: add #14754

* Changelog: add #14645

* Cahngelog: add #14599
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kraftbj kraftbj kraftbj approved these changes

Assignees

@jeherve jeherve

Labels
[Feature] Mobile Theme aka minileven [Pri] Normal [Type] Janitorial
Projects
None yet
Milestone
8.3
4 participants
@jeherve @jetpackbot @kraftbj @matticbot