Site Settings: Update the general tab to use toggles instead of checkboxes #10209

tyxla · 2016-12-21T14:47:06Z

Description

This PR:

Refactors the existing general settings to use toggles instead of checkboxes (see Implement Jetpack Module Settings #9171).
Removes the dirtyLinkedState mixin dependency.
Fixes a notice in <Timezone /> when no value is specified.
Implements i18n-calypso.localize for translated strings.

Inspired by @ryelle's awesome work in #10167.

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.
Make any changes.
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.
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

…State

matticbot · 2016-12-21T14:47:07Z

Test live: https://calypso.live/?branch=update/site-settings-general-toggles

rickybanister · 2016-12-21T15:00:25Z

Looks good to me.

rickybanister · 2016-12-21T15:01:23Z

I'll expand on that a bit—we're rightful in using toggles in situations where the choice is on/off binary (as it is in all of these cases). Let's just be sure to avoid swapping any 'multiple choice' checkboxes for toggles. Carry on!

MichaelArestad · 2016-12-21T15:09:16Z

This looks spot on. Nice work!

ryelle

Works great! Just left a few comments :)

ryelle · 2016-12-21T16:10:35Z

client/my-sites/site-settings/form-general.jsx

+		this.setState( { [ currentTargetName ]: currentTargetValue } );
+	},
+
+	handleToggle( name ) {


I wonder if we should move these functions into form-base, if we're going to have the same functions across a few tabs. Perhaps another PR after this & the discussion tab is merged.

I'd also consider renaming this to getToggleHandler.

@ryelle I was thinking the same thing! We should definitely do that, but let's do it in another PR after we have both these merged.

@omarjackman I'm fine both ways, but just so you know I've reused the same code that was implemented by @ryelle in #10167. Once we move these into form-base, we can consider renaming them.

ryelle · 2016-12-21T16:18:18Z

client/my-sites/site-settings/form-general.jsx

-						</FormLabel>
-					</li>
-				</ul>
+				<legend>{ translate( 'Holiday Snow' ) }</legend>


You could also use FormLegend, which adds the form-legend class.

ryelle · 2016-12-21T16:21:55Z

client/my-sites/site-settings/form-general.jsx

 				</FormLabel>

 				<Timezone
-					valueLink={ this.linkState( 'timezone_string' ) }
-					selectedZone={ this.linkState( 'timezone_string' ).value }
+					value={ this.state.timezone_string }


Is value used in this component? It looks like only selectedZone is used. I think even the valueLink that was here was ignored.

You're right! 👍

ryelle · 2016-12-21T16:31:29Z

client/my-sites/site-settings/form-general.jsx

 						disabled={ this.state.fetchingSettings }
 						onClick={ this.onRecordEvent( 'Clicked Site Visibility Radio Button' ) } />
-					<span>{ this.translate( 'Public' ) }</span>
+					<span>{ translate( 'Public' ) }</span>
 					<FormSettingExplanation isIndented>


Should this be indented? @MichaelArestad the grey text under each option

You raise a good question. My vote would be for indentation. I'll let @MichaelArestad chime in as well though.

It made me wonder how we might be able to ensure that radio buttons, checboxes, and small toggles (when left-aligned) might take up the same amount of space so that their respective labels would all share the same left alignment if the various form elements were mixed together in a list. Can we ensure that all three types of inputs fix within 24px and that text labels are always exactly 24px (or whatever it is) from the left justification?

If we strive to be consistent, I'd rather keep it non-indented because of how it is already in the Discussion tab in #10167 - see how the "These settings may be overriden..." text appears, and it's not indented. Anyway, let's also see what @MichaelArestad has to say about it. For now, I leave it indented.

Can we ensure that all three types of inputs fix within 24px and that text labels are always exactly 24px (or whatever it is) from the left justification?

Sure, but I think that for Radio buttons it would create a huge white gap. Mini toggles are 24px wide, and have 21px of space to the right, which totals 45px. Radio buttons (together with white space) are equal to 21px. If we add another 24px to the right, the white gap would be too big IMO.

ryelle · 2016-12-21T16:33:05Z

client/my-sites/site-settings/style.scss

+		margin-left: 12px;
+	}
+
+	p.form-setting-explanation.is-indented {


Regardless of whether FormSettingExplanation should be indented, this CSS should be removed. If you do end up needing to remove the indent, remove the isIndented property from the component.

Also, we probably shouldn't target the p tag directly

@ryelle of course, I've removed it now.

omarjackman

This looks awesome @tyxla. I left a few comments to take a look at.

omarjackman · 2016-12-22T01:14:00Z

client/my-sites/site-settings/form-general.jsx

+		if ( dirtyFields.indexOf( key ) === -1 ) {
+			newState.dirtyFields = [ ...dirtyFields, key ];
+		}
+		this.setState( newState );


Updating state here with no changes will still trigger a render. If the intention isn't specifically to do that then I think we should just return early

const dirtyFields = this.state.dirtyFields || []; if ( dirtyFields.indexOf( key ) >= 0 ) { return; } this.setState( { dirtyFields: [ ...dirtyFields, key ] } );

Also just noticed that instead of doing const dirtyFields = this.state.dirtyFields || []; we could just initialize the state with an empty array in getInitialState and then change this to
const { dirtyFields } = this.state;

You're bringing some good points here. Let's see what @ryelle's opinion about that (as I've borrowed this from her code in #10167), and I can later address them in this PR as well.

omarjackman · 2016-12-22T01:17:31Z

client/my-sites/site-settings/form-general.jsx

+
+	handleToggle( name ) {
+		return () => {
+			this.recordEvent.bind( this, `Toggled ${ name }` );


This line doesn't look like its doing anything. Did we just want to call it?

this.recordEvent( `Toggled ${ name }` );

Again, this has been borrowed from #10167, but it makes sense. I've removed it from this PR, but @ryelle might want to apply it to #10167 too.

omarjackman · 2016-12-22T01:31:04Z

client/my-sites/site-settings/form-general.jsx

+		this.setState( { [ currentTargetName ]: currentTargetValue } );
+	},
+
+	handleToggle( name ) {


I'd also consider renaming this to getToggleHandler.

omarjackman · 2016-12-22T01:33:54Z

client/my-sites/site-settings/style.scss

+		margin-left: 12px;
+	}
+
+	p.form-setting-explanation.is-indented {


Also, we probably shouldn't target the p tag directly

omarjackman · 2016-12-22T01:34:40Z

client/my-sites/site-settings/style.scss

+		flex: 1 0 auto;
+	}
+
+	.form-toggle__switch + span {


We should give this span a class and target it that way if at all possible.

Good point, I've added a class and styled it by that class. @ryelle you might want to apply this to #10167 as well.

… form

tyxla · 2016-12-22T09:20:09Z

@omarjackman @ryelle @rickybanister @MichaelArestad I've done some updates regarding the comments. This is ready for a new code and design review.

tyxla · 2016-12-22T12:27:52Z

A lot of the work (+ much more) has been addressed in #9085 which as already been merged.

It'll be a pain to merge all of these together, so I'll start a new PR for the toggle changes.

tyxla added 3 commits December 21, 2016 16:07

Site Settings: Replace checkboxes with toggles and remove dirtyLinked…

649d623

…State

Components: Use empty string instead of null for Timezone default value

14ab781

Site Settings: Use localize() for general settings form

3f716b3

tyxla added this to the Jetpack Module Settings in Calypso milestone Dec 21, 2016

tyxla self-assigned this Dec 21, 2016

tyxla requested review from rickybanister, ryelle, oskosk, MichaelArestad, johnHackworth and roccotripaldi December 21, 2016 14:47

oskosk mentioned this pull request Dec 21, 2016

Implement Jetpack Module Settings #9171

Closed

55 tasks

ryelle reviewed Dec 21, 2016

View reviewed changes

omarjackman reviewed Dec 22, 2016

View reviewed changes

tyxla added 3 commits December 22, 2016 10:41

Site Settings: Use FormLegend for holiday snow legend in General tab

cee4921

Site Settings: Remove obsolete value from Timezone component

057b04b

Site Settings: Cleanup obsolete CSS in general tab

14d8c42

tyxla mentioned this pull request Dec 22, 2016

Site Settings: Update the discussion tab to use separate cards #10167

Merged

tyxla added 2 commits December 22, 2016 11:07

Site Settings: Remove bind call from handleToggle in general settings…

a1f4cf1

… form

Site Settings: Add a classname to site settings toggle label

775cc15

tyxla closed this Dec 22, 2016

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 22, 2016

tyxla deleted the update/site-settings-general-toggles branch December 22, 2016 12:29

tyxla 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 22, 2016

tyxla mentioned this pull request Dec 22, 2016

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Site Settings: Update the general tab to use toggles instead of checkboxes #10209

Site Settings: Update the general tab to use toggles instead of checkboxes #10209

tyxla commented Dec 21, 2016 •

edited

Loading

matticbot commented Dec 21, 2016

rickybanister commented Dec 21, 2016

rickybanister commented Dec 21, 2016

MichaelArestad commented Dec 21, 2016

ryelle left a comment

ryelle Dec 21, 2016

omarjackman Dec 22, 2016 •

edited

Loading

tyxla Dec 22, 2016

ryelle Dec 21, 2016

ryelle Dec 21, 2016

tyxla Dec 22, 2016

ryelle Dec 21, 2016

rickybanister Dec 21, 2016

tyxla Dec 22, 2016 •

edited

Loading

ryelle Dec 21, 2016

omarjackman Dec 22, 2016

tyxla Dec 22, 2016

omarjackman left a comment

omarjackman Dec 22, 2016

omarjackman Dec 22, 2016

tyxla Dec 22, 2016

omarjackman Dec 22, 2016

tyxla Dec 22, 2016

omarjackman Dec 22, 2016 •

edited

Loading

omarjackman Dec 22, 2016

omarjackman Dec 22, 2016

tyxla Dec 22, 2016

tyxla commented Dec 22, 2016

tyxla commented Dec 22, 2016

Site Settings: Update the general tab to use toggles instead of checkboxes #10209

Site Settings: Update the general tab to use toggles instead of checkboxes #10209

Conversation

tyxla commented Dec 21, 2016 • edited Loading

Description

Screenshots

To test

matticbot commented Dec 21, 2016

rickybanister commented Dec 21, 2016

rickybanister commented Dec 21, 2016

MichaelArestad commented Dec 21, 2016

ryelle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omarjackman Dec 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tyxla Dec 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omarjackman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omarjackman Dec 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tyxla commented Dec 22, 2016

tyxla commented Dec 22, 2016

tyxla commented Dec 21, 2016 •

edited

Loading

omarjackman Dec 22, 2016 •

edited

Loading

tyxla Dec 22, 2016 •

edited

Loading

omarjackman Dec 22, 2016 •

edited

Loading