Skip to content

Writing Settings: disable Mobile Theme UI when feature isn't active#37857

Merged
jeherve merged 5 commits intomasterfrom
rm/minileven-ui
Dec 18, 2019
Merged

Writing Settings: disable Mobile Theme UI when feature isn't active#37857
jeherve merged 5 commits intomasterfrom
rm/minileven-ui

Conversation

@jeherve
Copy link
Copy Markdown
Member

@jeherve jeherve commented Nov 22, 2019 •
edited
Loading

Fixes #36404

Matching Jetpack PR:
Automattic/jetpack#14101

  • Internal references:
    • p1HpG7-84F-p2
    • p6TEKc-3iM-p2
    • p6TEKc-3jj-p2
    • p6TEKc-3l7-p2

Changes proposed in this Pull Request

The Mobile theme was developed 8 years ago, back when responsive themes weren't a thing. It isn't as useful today, and can even be harmful when one enables it on their site without realizing the consequences, as outlined in Automattic/jetpack#7602.

This PR comes as a first step to deprecate the Mobile Theme feature. We'll start by greying out Mobile Theme Settings from the Writing Settings UI when the feature is not active. Folks that still use the feature can access its settings and disable it. Folks who do not use it cannot turn it on via the UI.

In addition to those changes, we'll display a notice on top of the settings:

  • If the Mobile Theme feature is active on your site and if you run Jetpack 8.1 Alpha or newer, we'll show you a notice inviting you to check the look of the site on mobile devices if you did not run the Mobile Theme feature on your site. We can do so via a Google Tool.
  • If you run an older version of the Jetpack plugin, or if the Mobile Theme feature is already off on your site, you'll see a generic notice announcing the deprecation.

Module active

image

Module inactive

screenshot 2019-12-17 at 18 39 37

Testing instructions

  • Start from a site running Jetpack 8.1 Alpha.
  • Run yarn docker:wp jetpack module activate minileven to enable the module on your Jetpack site.
  • Go to Manage > Settings > Writing.
  • Notice that the Mobile Theme settings are there.
  • Notice the notice.
  • Click the 2 links to check the support docs as well as the Google checking tool.
  • Disable the feature.
  • The settings should become grey.

@jeherve jeherve added [Feature] Site Settings All other general site settings. Jetpack [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. Janitorial Mobile Web labels Nov 22, 2019
@jeherve jeherve self-assigned this Nov 22, 2019
@matticbot
Copy link
Copy Markdown
Contributor

matticbot commented Nov 22, 2019

Test live: https://calypso.live/?branch=rm/minileven-ui

@jeherve jeherve mentioned this pull request Nov 22, 2019
Merged
@matticbot
Copy link
Copy Markdown
Contributor

matticbot commented Nov 22, 2019 •
edited
Loading

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

Sections (~413 bytes added 📈 [gzipped])

Details 
name              parsed_size           gzip_size
settings-writing      +1096 B  (+0.2%)     +413 B  (+0.4%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

@ChaosExAnima
Copy link
Copy Markdown
Contributor

ChaosExAnima commented Nov 22, 2019

Can verify that I don't see it with this branch:

Screenshot from 2019-11-22 10-19-06

keoshi
keoshi approved these changes Nov 22, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@keoshi keoshi left a comment

Choose a reason for hiding this comment

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

Works great! 🚢

@jeherve jeherve added [Status] In Progress and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Nov 25, 2019
@jeherve
Copy link
Copy Markdown
Member Author

jeherve commented Nov 25, 2019

Moving this back to "in progress" as we may want to add a feature deprecation notice in there. See conversation in the Jetpack PR to find out more.

@jeherve
Copy link
Copy Markdown
Member Author

jeherve commented Dec 9, 2019

Noting that we may end up displaying things a bit differently.

Reference: p6TEKc-3iM-p2

@jeherve
Writing Settings: remove Mobile Theme UI when feature isn't active
462a48a 
Fixes #36404
@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 jeherve added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR and removed [Status] In Progress labels Dec 17, 2019
@jeherve jeherve requested a review from jeffgolenski December 17, 2019 17:44
jeffgolenski
jeffgolenski approved these changes Dec 17, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@jeffgolenski jeffgolenski left a comment

Choose a reason for hiding this comment

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

@jeherve Tested both inactive and active states. Looks great and both links work wonderfully.

@jeherve jeherve removed the [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR label Dec 18, 2019
@jeherve jeherve changed the title Writing Settings: remove Mobile Theme UI when feature isn't active Writing Settings: disable Mobile Theme UI when feature isn't active Dec 18, 2019
zinigor
zinigor approved these changes Dec 18, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@zinigor zinigor left a comment

Choose a reason for hiding this comment

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

Works well for me, the code looks good!

@jeherve jeherve merged commit 2937291 into master Dec 18, 2019
@jeherve jeherve deleted the rm/minileven-ui branch December 18, 2019 16:24
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Dec 18, 2019
@a8ci18n
Copy link
Copy Markdown

a8ci18n commented Jul 2, 2020

Translation for this Pull Request has now been finished.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jeffgolenski jeffgolenski jeffgolenski approved these changes

@keoshi keoshi keoshi approved these changes

+1 more reviewer

@zinigor zinigor zinigor approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jeherve jeherve

Labels

[Feature] Site Settings All other general site settings. Janitorial Jetpack Mobile Web

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Settings: Writing - Mobile Theme options not available in Calypso in simple sites as Atomic sites

7 participants

@jeherve @matticbot @ChaosExAnima @a8ci18n @jeffgolenski @zinigor @keoshi