Polish the checkbox, update switch documentation #8588

jasmussen · 2018-08-06T11:04:12Z

The switch has a long history. Before I go into details, here's what this PR does:

It replaces "Pending Review", "Stick to Front Page", "Allow Comments", and "Allow Pingbacks" from using Switches to using Checkboxes.
It tweaks the design of the checkbox to have a significantly more visible checked state, and a new more visual focus state.
It tweaks the documentation for when to use a switch instead of a checkbox.
It unifies the margins of checkboxes to use our 4/8/12 grid, and adds a little padding.

The Switch is born from mobile, which is increasingly the device of choice for people and long term might become the only choice. It's bigger and more tappable, and implies by its nature that it's a boolean choice. It also has qualities that the checkbox does not embody:

Like its wall lightswitch counterpart, it’s something you flip and the effect is instant. There used to be darkness, now there’s light.
It has only a single purpose, and that purpose is boolean. For example you wouldn’t use switches to select multiple categories.

However "Allow Comments" and the three other aforementioned toggles that use switches in master, do not have an instant effect, they require you to hit "Save" before the toggle option is stored. Given the above reasoning, a checkbox is a better choice in those situations.

The toggle is still a good choice in situations where immediate visual feedback is given, like when a dropcap is applied, or when you toggle image croppping for gallery items. In those situations, the pairing of the boolean nature and the larger touch target makes the switch feel more substantial.

With this PR I'm trying to take into account all the past discussions on the topic, and I know there have been many. Lay your thoughts on me.

Screenshots, checking and focus styles:

Relaxed margins:

Instant effect toggles:

youknowriad · 2018-08-06T11:09:05Z

edit-post/assets/stylesheets/main.scss

@@ -168,6 +168,32 @@ body.gutenberg-editor-page {
 			outline-offset: 0;
 		}
 	}
+
+	input[type=checkbox] {


Should we move these styles to the "Checkbox control" or do we want them to apply globally to checkboxes?

Moving to CheckboxControl means, it works outside Gutenberg, outside WordPress, whenever you use the component

It also means that if you use a checkbox with the CheckboxControl component, it won't have the styles.

So each option has pros/cons.

Shoot, I thought I commented on this but didn't hit submit. Was gonna say the same thing, I don't think this should be a global checkbox style. We should scope it to our component and just make sure we're using the CheckboxControl.

The presence here is intentional in the same way as the custom styles for input fields here are intentional. They are new designs for these base ingredients, not just the components. In a way the presence here is tied to #8588 (comment), which is — these should be styles for all of WordPress.

Also, I initially had it in the checkbox control, but then it did not apply to categories.

Should we have these styles in both places? If there's a JavaScripty future for the rest of WordPress, we could imagine one day having these styles live only in the Checkbox component and every checkbox in WordPress would use that component. But in the mean time, what's the best approach?

If there's a JavaScripty future for the rest of WordPress

I think there is 😄

I think we should try to contain our styles and apply them only to our components. We should simply get more things using our components, but the benefits of component-based development is that we can scope styles to an easily-reusable component and then just use it everywhere.

I'd really prefer we scope the styles to this component.

@tofumatt I appreciate your passion for code cleanliness. I share it. My favorite t-shirt is one my friend @MichaelArestad wore a few years ago, that spells "Hella Clean Code" with sparkles around it.

In this case, however, if I scope the checkbox component to be just scoped to the component, the categories aren't styled:

I'm guessing that can be fixed by actually using the checkbox control as part of the categories component, and that this in turn would be the right thing to do. But you'll notice that the input styles starting on this line are also located in this very same file. Should they also be moved to components? And in turn, which input styles will then break if we do this?

I think an argument can be made that the these form widgets, input, select, checkboxes, etc. are sort of base ingredients that should be styled in a global stylesheet, as opposed to on a scoped component basis. Or, should each of those HTML elements be a component on its own? And if so, is that a separate PR?

I'm not trying to be difficult — truly my biggest goal here is to simply make sure that all the textfields and checkboxes inside Gutenberg look consistent, and longer term that those same styles can be removed from Gutenberg because they are now default in WordPress as a whole (hence the need for the upstream tickets).

What's the best path forward?

Hmmm... okay. So I'd say that what we really want is:

styles are *always scoped to a component

we use a component for everything (even making our own checkbox component so it has styles)

That said, you're right for now: it's too much of a pain to do that and it's out of scope.

Your PR is an improvement. Let's not let me make perfect the enemy of great! 😄

We'll make that vision come true. I share it too. One step at a time we'll get there. Thanks Matt!

tofumatt

I largely dig it, I think a few tweaks would be nice but I'll push a few things and let you respond to feedback... after that it's good to go.

tofumatt · 2018-08-06T11:09:08Z

edit-post/assets/stylesheets/main.scss

@@ -121,13 +121,13 @@ body.gutenberg-editor-page {
 	}
 }

-// Override core input styles to provide ones consistent with Gutenberg
-// @todo submit as upstream patch as well
+// Override core input styles to provide ones consistent with Gutenberg.


"ones" is a slightly ambiguous word here; it'd say "styles" would be better.

tofumatt · 2018-08-06T11:09:45Z

edit-post/assets/stylesheets/main.scss

-// Override core input styles to provide ones consistent with Gutenberg
-// @todo submit as upstream patch as well
+// Override core input styles to provide ones consistent with Gutenberg.
+// @todo Submit as upstream patch as well.


Can you file an issue for this please and link to it? These TODOs will get lost to time otherwise 😄

At least if there's an issue it'll get lost to time but be tracked! 😉

So far I've intentionally not filed, because the styles for these components were partially in flux. But as of the most recent input style change, and now this, things are solidifiying. How about this: I await a little feedback on this branch, and if the thumbs point upwards, I will create the ticket, update the PR with a link in the comment, and we'll go from there?

Works for me. In general I say more issues (especially things like this which are easily marked as "code quality" issues we can filter out). Easier to close unneeded ones than remember long-forgotten ones 😉

tofumatt · 2018-08-06T11:10:31Z

edit-post/assets/stylesheets/main.scss

@@ -168,6 +168,32 @@ body.gutenberg-editor-page {
 			outline-offset: 0;
 		}
 	}
+
+	input[type=checkbox] {
+		border: 2px solid $dark-gray-300;


That hardcoded border width makes me nervous. Do we want a rule that checkboxes have double the usual border width? I dunno.

Hardcoded makes it sound so serious :)

The double border is to give weight to the checkbox, without making it overwhelming. Visually I like the balance but design feedback may change this. What would be good CSS if/when the visuals are settled?

Yeah that makes sense. I just think either a variable named 2px or even just using $border-width + 1 would work? That's what this is... and it makes sense to base it off our global borders.

tofumatt · 2018-08-06T11:11:08Z

edit-post/assets/stylesheets/main.scss

+			box-shadow: 0 0 0 1px $dark-gray-300;
+		}
+
+		&:checked:focus {


This would be more readable if it were underneath the plain :checked style below. I think it's nice to "stack" styles like that.

Pushing a fix in a second, let me know if that actually addresses your comment, wasn't sure I understood you perfectly.

tofumatt · 2018-08-06T11:13:16Z

edit-post/assets/stylesheets/main.scss

+			border-color: theme( toggle );
+		}
+
+		&:before {


Isn't this selector technically supposed to be ::before? I know :before works but we should use ::before: https://developer.mozilla.org/en-US/docs/Web/CSS/::before#Syntax

We should probs have a rule for it 🤷‍♂️

It is, and I know, and I'm pretty sure we've discussed this. The reason I went with single colon (besides habits, but habits can change, you've seen this from me capitalizing and period ending my comments now), is that single colon is what we've used EVERYWHERE else in the code. If we add a linter rule and convert those all to double colons, that could probably be a good separate PR no?

That's legit. Filed #8618.

tofumatt · 2018-08-06T11:13:56Z

packages/components/src/form-toggle/README.md

@@ -1,5 +1,21 @@
 # FormToggle

+The Form Toggle Control (or Switch) is a complement to the Checkbox Control, which is used when the effect is instant and boolean. Like its light switch counterpart that brings light, when you flip this switch something changes instantanously.


I have a few tweaks to make here but I'll just push a commit 😄

tofumatt · 2018-08-06T11:15:31Z

Also, thanks for the fantastic PR summary, really made this one a breeze to review 😄

paulwilde · 2018-08-06T11:51:16Z

By changing them to using the CheckboxControl the withInstanceId can now be dropped from these components to clean things up further.

tofumatt · 2018-08-06T13:29:22Z

(Really sorry but I got a bit distracted by another branch so I'll get back to this in a few hours, hope that's okay.)

jasmussen · 2018-08-06T13:33:18Z

No worries, I'm about to push a few fixes based on your feedback so maybe hold off a minute, just npm installing a thing.

jasmussen · 2018-08-06T13:36:18Z

Okay, pushed a fix for some of the comments, be sure to pull if you wanted to change the documentation!

tofumatt · 2018-08-06T18:40:13Z

Thanks, sorry I got distracted, my brain is scattered all over the map today.

karmatosed

I really like this! Thanks for tackling this issue.

tofumatt

I pushed my docs tweak, but I think we should be scoping our styles and I wonder about that TODO now that design feedback is addressed.

So I think a few things here need to be done before we merge, but largely I dig it. Ping me if I was unclear about anything.

afercia · 2018-08-07T06:58:40Z

For reference, there's a pending ticket for core (3 years) proposing to improve the checkboxes (and radio buttons) contrast ratio. This new Gutenberg design seems to be a very good improvement. https://core.trac.wordpress.org/ticket/35596

jasmussen · 2018-08-07T09:22:16Z

Rebased, squashed, addressed border feedback and fixed a few typos.

Next I'm going to look at creating the upstream ticket for the other components, link that as a comment.

jasmussen · 2018-08-07T11:15:34Z

I created https://core.trac.wordpress.org/ticket/44749 to track the new styles, and replaced the todos in 9d3aa18.

Thanks for the reviews everyone, I will merge if the tests pass.

jasmussen · 2018-08-07T11:22:00Z

Just realized I'd missed a bit of feedback. One last sanity check of 544fc84 please thank you?

jasmussen · 2018-08-07T17:13:50Z

The tests pass for me locally, but I'm not sure why travis keeps failing. I've tried restarting it twice now. Any insights?

jasmussen · 2018-08-08T10:22:35Z

Upon reinstalling my entire local dev environment, I got a few lovely new linter tests along for the ride, and they revealed a few syntax changes that may have caused the build to fail. It's now fixed, rebased, and I'm expecting the tests to pass.

The switch has a long history. Before I go into details, here's what this PR does: - It replaces Pending Review, Stick to Front Page, Allow Comments, and Allow Pingbacks from using Switches to using Checkboxes. - It tweaks the design of the checkbox to have a significantly more visible checked state, and a new more visual focus state. - It tweaks the documentation for when to use a switch instead of a checkbox. The Switch is born from mobile, which is increasingly the device of choice for people and long term might become the only choice. It's bigger and more tappable, and implies by its nature that it's a boolean choice. It also has qualities that the checkbox does not embody: - Like its wall lightswitch counterpart, it’s something you flip and the effect is instant. There used to be darkness, now there’s light. - It has only a single purpose, and that purpose is boolean. For example you wouldn’t use switches to select multiple categories. However Allow Comments and the three other aforementioned toggles that use switches in master, do not have an instant effect, they require you to hit "Save" before the toggle option is stored. Given the above reasoning, a checkbox is a better choice in those situations. The toggle is still a good choice in situations where immediate visual feedback is given, like when a dropcap is applied, or when you toggle image croppping for gallery items. In those situations, the pairing of the boolean nature and the larger touch target makes the switch feel more substantial. With this PR I'm trying to take into account all the past discussions on the topic, and I know there have been many. Lay your thoughts on me.

@paulwilde

Thanks for the feedback, @paulwilde.

jasmussen · 2018-08-08T12:02:11Z

I'm getting this e2e test:

But I'm not sure why that should be affected by this branch, as it's mostly touching CSS and a few panels in the inspector. Is this failing in master too?

This is failling too:

This one looks legit:

Not sure if this is kosher, as the ID will change if additional checkboxes are added.

gziolo · 2018-08-08T12:50:10Z

test/e2e/specs/writing-flow.test.js fails also locally for me on the latest master branch. Let's merge it as is 👍

jasmussen added [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Needs Design Feedback Needs general design feedback. labels Aug 6, 2018

jasmussen self-assigned this Aug 6, 2018

jasmussen requested a review from a team August 6, 2018 11:04

youknowriad reviewed Aug 6, 2018

View reviewed changes

tofumatt reviewed Aug 6, 2018

View reviewed changes

tofumatt self-assigned this Aug 6, 2018

tofumatt mentioned this pull request Aug 6, 2018

Use ::before (pseudo elements) instead of :before (pseudo classes) #8618

Closed

karmatosed self-requested a review August 6, 2018 21:09

karmatosed approved these changes Aug 6, 2018

View reviewed changes

tofumatt removed the Needs Design Feedback Needs general design feedback. label Aug 6, 2018

tofumatt requested changes Aug 6, 2018

View reviewed changes

jasmussen force-pushed the try/checkbox-switch-improvements branch from 955f8ef to a8f5319 Compare August 7, 2018 09:19

tofumatt approved these changes Aug 7, 2018

View reviewed changes

jasmussen added this to the 3.5 milestone Aug 7, 2018

jasmussen force-pushed the try/checkbox-switch-improvements branch from 544fc84 to ae685de Compare August 7, 2018 12:31

jasmussen force-pushed the try/checkbox-switch-improvements branch from ae685de to 4d41ff5 Compare August 8, 2018 10:14

tofumatt and others added 2 commits August 8, 2018 13:47

docs: Tidy up FormToggle docs

de5f8b9

Removed unused variables and withInstanceId.

589eeec

Thanks for the feedback, @paulwilde.

jasmussen force-pushed the try/checkbox-switch-improvements branch from cd57342 to 589eeec Compare August 8, 2018 11:47

jasmussen added 2 commits August 8, 2018 14:17

Try a fix for the legitimate failing e2e test.

8a0e23d

Not sure if this is kosher, as the ID will change if additional checkboxes are added.

Fix e2e test really.

957fb24

gziolo merged commit ee6060f into master Aug 8, 2018

gziolo deleted the try/checkbox-switch-improvements branch August 8, 2018 12:50

diegoreymendez mentioned this pull request Aug 8, 2018

Publishes unknown-type handler methods for mobile. #8688

Merged

4 tasks

aduth mentioned this pull request Aug 10, 2018

Update plugin version to 3.5 #8784

Merged

chrisvanpatten mentioned this pull request Aug 10, 2018

Apply new checkbox CSS to radio buttons and fix border radius #8835

Merged

4 tasks

ZebulanStanphill mentioned this pull request Aug 10, 2018

Checkbox styles are inconsistent #8825

Closed

chrisvanpatten mentioned this pull request Oct 3, 2018

"Stick to the Front Page" should use a toggle instead of a checkmark #10290

Closed

dependabot-preview bot mentioned this pull request Dec 31, 2019

Bump @wordpress/i18n from 2.0.0 to 3.7.0 wearerequired/rest-likes#97

Closed

dependabot-preview bot mentioned this pull request Feb 4, 2020

Bump @wordpress/jest-console from 3.4.0 to 3.5.0 tdreyno/druyan#215

Merged

dependabot-preview bot mentioned this pull request Apr 15, 2020

Bump @wordpress/babel-plugin-makepot from 3.4.0 to 3.5.0 swissspidy/gutenberg-i18n-block#93

Merged

		@@ -1,5 +1,21 @@
		# FormToggle

		The Form Toggle Control (or Switch) is a complement to the Checkbox Control, which is used when the effect is instant and boolean. Like its light switch counterpart that brings light, when you flip this switch something changes instantanously.

Polish the checkbox, update switch documentation #8588

Polish the checkbox, update switch documentation #8588

Conversation

jasmussen commented Aug 6, 2018 • 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

tofumatt 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

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

Choose a reason for hiding this comment

tofumatt commented Aug 6, 2018

paulwilde commented Aug 6, 2018 • edited Loading

tofumatt commented Aug 6, 2018

jasmussen commented Aug 6, 2018

jasmussen commented Aug 6, 2018

tofumatt commented Aug 6, 2018

karmatosed left a comment

Choose a reason for hiding this comment

tofumatt left a comment

Choose a reason for hiding this comment

afercia commented Aug 7, 2018

jasmussen commented Aug 7, 2018

jasmussen commented Aug 7, 2018 • edited Loading

jasmussen commented Aug 7, 2018

jasmussen commented Aug 7, 2018

jasmussen commented Aug 8, 2018

jasmussen commented Aug 8, 2018 • edited Loading

gziolo commented Aug 8, 2018

jasmussen commented Aug 6, 2018 •

edited

Loading

paulwilde commented Aug 6, 2018 •

edited

Loading

jasmussen commented Aug 7, 2018 •

edited

Loading

jasmussen commented Aug 8, 2018 •

edited

Loading