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

Mobile theme: remove unused option that prevented it from loading #4866

Merged
merged 1 commit into from Aug 19, 2016

Conversation

eliorivero
Copy link
Contributor

@eliorivero eliorivero commented Aug 17, 2016

Fixes p1HpG7-3s1-p2

Changes proposed in this Pull Request:

  • Removes the option wp_mobile_disable and an associated filter option_wp_mobile_disable that prevented it from loading.

Testing instructions:

  • Activate mobile theme
  • Visit the site in desktop, it should use the regular theme
  • Visit the site in mobile, it should use the Minileven mobile theme

@eliorivero eliorivero added [Type] Bug When a feature is broken and / or not performing as intended [Feature] Mobile Theme aka minileven [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Aug 17, 2016
@eliorivero eliorivero added this to the 4.2.2 milestone Aug 17, 2016
@kraftbj
Copy link
Contributor

kraftbj commented Aug 18, 2016

Introduced in ad38724 via #4281 by autoloading the option, which was previously not set.

If we ever trying to sync up minileven between Jetpack and WP.com (ignored in the build script since r97888-wpcom / 2014), we'll run into problems since on WP.com, you can disable the mobile theme (since this runs as a mu-plugin there).

That said, since we haven't been syncing it and my guess that we'll care less and less about the mobile theme given responsive theming, probably safe.

cc: @gravityrail

@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 Aug 18, 2016
@@ -34,8 +34,6 @@ function jetpack_check_mobile() {
return false;
if ( jetpack_mobile_exclude() )
return false;
if ( 1 == Jetpack_Options::get_option_and_ensure_autoload( 'wp_mobile_disable', '0' ) )
Copy link
Contributor

@kraftbj kraftbj Aug 18, 2016

Choose a reason for hiding this comment

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

In addition to straight removing this, should we clean up after ourselves and delete wp_mobile_disable since it'll be set on all 4.2 sites?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wouldn't. In sites < 4.2 this option wouldn't even be set. In sites >= 4.2, it's neglectable. The thing is that we would have to add some checking to see if the option was created or not and delete it, and don't want to add more processing time to clean up something so small.

@jeherve
Copy link
Member

jeherve commented Aug 18, 2016

@eliorivero
Copy link
Contributor Author

@kraftbj thanks for the feedback. I've changed this PR and decided to add the filter only in wpcom so in this way the feature continues to work normally in the plugin version and we're covered if we ever sync this file back to wpcom. I think we're good to go after this but can you please review it?

@kraftbj
Copy link
Contributor

kraftbj commented Aug 18, 2016

Looking at the build script, the files within /modules/minileven are commented out, but modules/minileven.php isn't mentioned at all. I think we're safe to remove it from that file completely, but no harm in the conditional either.

@annezazuu
Copy link

mentioned in 2783445-t, 2783483-t, 2783582-t , 2782687-t

@scarstocea
Copy link

Mentioned in 2782854-t.

@samhotchkiss samhotchkiss merged commit e34443b into master Aug 19, 2016
@samhotchkiss samhotchkiss removed the [Status] Ready to Merge Go ahead, you can push that green button! label Aug 19, 2016
@kraftbj kraftbj deleted the fix/mobile-theme branch August 20, 2016 04:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Mobile Theme aka minileven [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants