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

Jetpack Settings: Add Theme Enhancements card under Writing Settings for Jetpack sites #10083

Merged
merged 22 commits into from
Jan 24, 2017

Conversation

tyxla
Copy link
Member

@tyxla tyxla commented Dec 15, 2016

Introduction

This PR is part of #9171. It introduces a Theme Enhancements card under Writing Settings. This card will be used to manage the settings of the Infinite Scroll and Mobile Theme Jetpack modules.

Preview

Both modules activated, some settings are disabled

Initial loading

Loading while saving

Disabled modules (settings are hidden)

Testing

  • Checkout this branch.
  • Select one of your Jetpack sites with Jetpack 4.5 or newer.
  • Go to /settings/writing/$site, where $site is the slug of your Jetpack site.
  • Verify the Theme Enhancements card is displayed properly.
  • Play with the main toggles that enable the modules, and verify they save correctly in your Jetpack site (note that notices are implemented separately in Jetpack Settings: Add notices for (de)activating Jetpack modules #10583).
  • Verify that the settings of a module are displayed only when the module is active.
  • Play around with the settings of each module and click "Save Settings". Verify they save properly in your Jetpack site.
  • Verify the links of the info popovers are working as expected and lead to the right place.
  • Test with a Jetpack site that has Jetpack 4.4.2 or older, and verify the card is not shown.
  • Test with a WordPress.com site and verify that the card is not shown.

@tyxla tyxla added this to the Jetpack Module Settings in Calypso milestone Dec 15, 2016
@tyxla tyxla self-assigned this Dec 15, 2016
@matticbot
Copy link
Contributor

@oskosk oskosk mentioned this pull request Dec 15, 2016
55 tasks
@tyxla tyxla force-pushed the add/jetpack-setings-theme-enhancements-card branch from b67c60b to 2b944d9 Compare December 16, 2016 09:00
@beaulebens
Copy link
Member

Can we get a screenshot/mock of what we should be seeing for this?

@beaulebens
Copy link
Member

screen shot 2016-12-22 at 2 29 15 pm

You might not have done this one yet, but this appears to be the wrong helper/information link for this setting.

@tyxla tyxla force-pushed the add/jetpack-setings-theme-enhancements-card branch from c3a1773 to 6dd156c Compare December 23, 2016 08:37
@tyxla
Copy link
Member Author

tyxla commented Dec 23, 2016

Can we get a screenshot/mock of what we should be seeing for this?

Sure thing, I've updated the PR description to contain a screenshot. I'll also update with some testing instructions and additional information once the PR is ready to review.

You might not have done this one yet, but this appears to be the wrong helper/information link for this setting.

Yeah, this was in progress, but I've now updated it, too. Could have missed it, so good catch! 👍

@tyxla tyxla force-pushed the add/jetpack-setings-theme-enhancements-card branch 3 times, most recently from b39f180 to 165e2dc Compare December 23, 2016 14:13
@tyxla
Copy link
Member Author

tyxla commented Dec 28, 2016

This PR works and saves data properly; however it will be best if we adapt it to using the bulk settings endpoint; this is being implemented in #10123.

@tyxla tyxla force-pushed the add/jetpack-setings-theme-enhancements-card branch from 165e2dc to 783d7fb Compare January 5, 2017 09:32
@tyxla
Copy link
Member Author

tyxla commented Jan 5, 2017

I've adapted this PR to use bulk settings from #10123, and done a ton of other improvements. I've also updated the PR description with new screenshots, and a TODO list with the remaining stuff.

@tyxla tyxla force-pushed the add/jetpack-setings-theme-enhancements-card branch from d20ad50 to 315c158 Compare January 5, 2017 14:33
@tyxla tyxla force-pushed the add/jetpack-setings-theme-enhancements-card branch 4 times, most recently from 87e0b5c to af70a7b Compare January 13, 2017 08:38
@tyxla tyxla added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Type] Task and removed [Status] In Progress labels Jan 13, 2017
Copy link
Member

@roccotripaldi roccotripaldi left a comment

Choose a reason for hiding this comment

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

Code looks good. Everything works smoothly, and as described, even when switching between sites.

@roccotripaldi roccotripaldi added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jan 23, 2017
tyxla added 22 commits January 24, 2017 10:01
@tyxla tyxla force-pushed the add/jetpack-setings-theme-enhancements-card branch from 78b942b to 4b138de Compare January 24, 2017 08:12
@tyxla tyxla merged commit de80da4 into master Jan 24, 2017
@tyxla tyxla deleted the add/jetpack-setings-theme-enhancements-card branch January 24, 2017 08:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants