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

Site Settings: Convert checkboxes to toggles in the General tab #10229

Merged
merged 1 commit into from
Dec 22, 2016

Conversation

tyxla
Copy link
Member

@tyxla tyxla commented Dec 22, 2016

This PR refactors the existing general settings to use toggles instead of checkboxes (see #9171). Inspired by @ryelle's awesome work in #10167. This work was previously done in #10209, but I had to begin from scratch because 90% of what I did was done there was done in #9085 (which was recently merged).

Screenshots

Jetpack Site

WordPress.com site

To test

  • Get this branch going locally or on calypso.live.
  • Visit the General settings at /settings/general/$site for a Jetpack site.
  • Note that in order to see the post status sync option, you need Jetpack <= 4.1.1 on your Jetpack site.
  • Make any changes to any of the toggles.
  • Verify that all changes have been saved and applied to your Jetpack site.

Also, test on WordPress.com sites (it contains Language and Site Timezone options, as well as a Private option for the Privacy setting):

  • Get this branch going locally or on calypso.live.
  • Visit the General settings at /settings/general/$site for a WordPress.com site.
  • Make any changes to any of the toggles.
  • Verify that all changes have been saved and applied to your WordPress.com site (you can also check the production settings page).

/cc
@ryelle @oskosk @roccotripaldi @johnHackworth for code review
@rickybanister @MichaelArestad for design review

@tyxla tyxla added the [Feature] Site Settings All other general site settings. label Dec 22, 2016
@tyxla tyxla added this to the Jetpack Module Settings in Calypso milestone Dec 22, 2016
@tyxla tyxla self-assigned this Dec 22, 2016
@matticbot
Copy link
Contributor

@tyxla tyxla added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Dec 22, 2016
@oskosk oskosk mentioned this pull request Dec 22, 2016
55 tasks
@tyxla tyxla added 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 22, 2016
@oskosk
Copy link
Contributor

oskosk commented Dec 22, 2016

This looks good to me but a few observations:

  1. In the context of the new designs for settings, toggles are proposed to save as soon as they toggle.
    From p6TEKc-HQ-p2:

_When a toggle is used, it should immediately enable that thing and show a global notice to show status of enabling that thing.

  1. This PR will make the General tab checkboxes consistent with the design of the other tabs, but we have no mention about how the General tab should look in p6TEKc-HQ-p2. We could use an opinion from @rickybanister about this.

@tyxla
Copy link
Member Author

tyxla commented Dec 22, 2016

@oskosk the toggles were already approved from @rickybanister and @MichaelArestad in the other PR (#10209), so I think just a general design feedback is necessary on this one.

@oskosk
Copy link
Contributor

oskosk commented Dec 22, 2016

Alirght!, then :shipit:

@oskosk oskosk removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Dec 22, 2016
@MichaelArestad
Copy link
Contributor

In the context of the new designs for settings, toggles are proposed to save as soon as they toggle.

@oskosk is right about the functionality, but let's fix that in another PR. Looks good on design.

@MichaelArestad MichaelArestad added [Status] Ready to Merge and removed [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR labels Dec 22, 2016
@tyxla
Copy link
Member Author

tyxla commented Dec 22, 2016

Thanks for the quick review folks 👍

@tyxla tyxla merged commit 89975ea into master Dec 22, 2016
@tyxla tyxla deleted the update/site-settings-general-checkboxes-to-toggles branch December 22, 2016 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Site Settings All other general site settings.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants