-
Notifications
You must be signed in to change notification settings - Fork 800
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: do not display settings when module is inactive. #14101
Conversation
Thank you for the great PR description! When this PR is ready for review, please apply the Scheduled Jetpack release: January 14, 2020. |
Code works, but the transition is very abrupt- it immediately vanishes when I click off. Can we have it so that it's there till the user refreshes the page, perhaps? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works as expected in my testing!
That's a good point. I am not sure what the best approach would be for this, though. Should we fake the deactivation until they move to a different page, or tab? |
What if instead of a workaround to leave it for the page session, we just add a line of text to the card - something like:
|
See discussion here: #14101 (comment)
Done in afeb464. @jeffgolenski How does this look to you? You should be able to test on Jurassic Ninja as soon as the tests pass on this PR. 👍 |
@jeherve That looks good to me. I'd personally like to have a little more visual call-out for this, but I don't think it's worth adding extra code to a component just for this notice. 🚢 |
I agree with @ChaosExAnima that the transition is harsh, but given the overall intent to remove the feature soon, I'm not going to block merging based on that. |
See discussion here: #14101 (comment)
Also use standard notice component to display notice.
326a89f
to
9f59a81
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested with both the theme already active, and inactive. Works great. Let's ship it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. No errors in the console.
* 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 <[email protected]>
* 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 <[email protected]>
Fixes #13385
Fixes #7602
Matching Calypso PR: Automattic/wp-calypso#37857
Changes proposed in this Pull Request:
This PR comes as a first step to deprecate the Mobile Theme feature. We'll start by hiding Mobile Theme Settings from the Jetpack 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.
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 #7602.
Is this a new feature or does it add/remove features to an existing part of Jetpack?
Testing instructions:
yarn docker:wp jetpack module activate minileven
to enable the module on your site.localStorage.setItem( 'debug', 'dops:analytics' );
to your browser's console.Proposed changelog entry for your changes: