Polish tertiary actions, switches, save state #10552

jasmussen · 2018-10-12T12:49:11Z

This PR does a few things. I know how you so love those, @mtias.

First, it tweaks the save draft/switch to draft area:

This addresses feedback from #10415, #9452, #9455.

Secondly, it makes the "Trash" button into a normal button, but colored red:

This is part of ongoing "align things to a grid" efforts, and sort of reduces the cognitive load of the whole panel.

Thirdly, it moves the switch to the left of the label:

This is also part of those "align things to a grid" efforts, as we generally have both icons, checkboxes, and other items to the left of labels. Now the wwitch is too.

Your thoughts are appreciated.

youknowriad · 2018-10-12T12:55:02Z

I love the consistency it brings to the "Save draft" and the "Switch to draft" buttons. Also, I like that the distinction between those buttons and the "saved" state is more visible.

Design wise, I prefer the new trash button over the link we had.

Just a small note on the PR, I'd have preferred separate PRs for each part (save draft, trash button and the toggles) as they seem not related, it eases reviews.

youknowriad · 2018-10-12T12:55:40Z

Question? What about mobile? Do we still show a "save" icon on mobile?

jasmussen · 2018-10-12T12:57:18Z

Just a small note on the PR, I'd have preferred separate PRs for each part (save draft, trash button and the toggles) as they seem not related, it eases reviews.

I did it again, didn't I? I officially owe Matías a milkshake. Sorry about that.

Question? What about mobile? Do we still show a "save" icon on mobile?

Yes, that icon is simply hidden beyond the mobile viewpoint. On mobile it's still there.

I would suggest that the way to retire the cloud icon on mobile, is to improve the auto-save feature to the point where we can actually retire it as a disparate action.

mtias · 2018-10-12T13:14:47Z

I did it again, didn't I? I officially owe Matías a milkshake. Sorry about that.

I keep a notebook with all these. It's kind of full.

aduth · 2018-10-12T14:29:20Z

What is TextButton? This separation appears to move us further away from semantic improvements discussed in #7534 / #9702.

alexislloyd · 2018-10-12T14:37:46Z

I would suggest that the way to retire the cloud icon on mobile, is to improve the auto-save feature to the point where we can actually retire it as a disparate action.

Agreed. In the meantime, is the issue that the cloud icon is unclear? If so, this is another material icon for saving that gets away from either the floppy disk or the cloud. Might be a solid alternative for the time being.

jasmussen · 2018-10-12T14:52:05Z

In the meantime, is the issue that the cloud icon is unclear? If so, this is another material icon for saving that gets away from either the floppy disk or the cloud. Might be a solid alternative for the time being.

Yes, there's an issue being discussed here: #9455

Merging this PR will actually close that issue, but be sure to reopen it if need be.

jasmussen · 2018-10-12T14:57:47Z

@aduth

What is TextButton? This separation appears to move us further away from semantic improvements discussed in #7534 / #9702.

Good points.

For now, I've retired the new separate TextButton component, and instead made it a isText prop.

I think the semantic aspects you bring up in those two tickets make sense, and the "link" prop should probably be retired. But I also think there's value in having three levels of importance for the buttons — primary, secondary, tertiary — but otherwise be relatively identical in function.

How do you feel about the isText prop?

aduth · 2018-10-12T15:27:32Z

Forcing myself into a perspective of ignorance by avoiding looking at the context in which it's implemented: I have no idea what is implied in a "text variation" of a button.

Is it similar to how we have an optional link aesthetic (cosmetic) to a button? If so, what's the difference between it and being a disabled link?

To me, text is not interactable, so the idea of a text button is paradoxical.

aduth · 2018-10-12T15:33:53Z

But I also think there's value in having three levels of importance for the buttons — primary, secondary, tertiary

So why not reflect this in the prop name, e.g. isPrimary, isSecondary, isTertiary, rather than its visual appearance, which as best I can tell is an incidental detail.

mtias · 2018-10-12T15:56:39Z

I'm not sure there is a tertiary / secondary distinction there, but could work.

aduth · 2018-10-12T16:00:35Z

In any case, we already have an isPrimary prop for Button, which should be reconciled for consistency if we're going to opt for prop naming by appearance than by semantic purpose / hierarchy.

mtias · 2018-10-12T16:05:47Z

We should definitely clear these component semantics.

jasmussen · 2018-10-12T16:35:08Z

There are a few layers to it. The plain "text" button is a concept widely used in mobile contexts as well, for an action that isn't a hyperlink but also isn't super important. It's often referred to there as a Text Button. See also https://material.io/design/components/buttons.html#usage.

But I like the idea of calling it tertiary, and I wouldn't be opposed to exploring ways to make it look less like a hyperlink. This could involve bolder text, or a different color — though I really do like that the color is the same as the color used on the Primary button.

ZebulanStanphill · 2018-10-12T17:54:38Z

Secondly, it makes the "Trash" button into a normal button, but colored red:

I don't like this. Move to trash is a link, so it should be styled like one. We should be moving towards more semantic distinction between buttons and links, not confusing them more.

Speaking of which, the clickable values for Visibility and Publish are styled like links, but they are buttons. I think they should be changed to look like what you have made Save Draft look like.

Other than that, I think I like these changes.

youknowriad · 2018-10-12T19:02:50Z

I don't like this. Move to trash is a link, so it should be styled like one.

Actually, I had the same impression but after checking, it's a button not a link, it doesn't have any "href".

ZebulanStanphill · 2018-10-12T19:18:49Z

@youknowriad Huh, you're right. It's actually a <button>. I guess the style change is fine, then.

robertdall · 2018-10-13T18:21:50Z

In the meantime, is the issue that the cloud icon is unclear? If so, this is another material icon for saving that gets away from either the floppy disk or the cloud. Might be a solid alternative for the time being.

Yes, there's an issue being discussed here: #9455

Merging this PR will actually close that issue, but be sure to reopen it if need be.

The #9455 hasn't been resolved yet and I hesitate to use anything that looks like a download/cloud button as we are not downloading anything we are saving the post to the server, not the user's device.

karmatosed

From a design perspective let's get this approved. One point I am on fence about is the trash as a button. I agree with taking away the icon but as far as making a button goes, this isn't the same as other places in WP core. However, we can have this in if people agree it's the right route forward. I do agree with removing the icon though.

Assuming we have the button in for trash, potentially we need to explore other places for this to bring in over time.

gziolo · 2018-10-16T10:17:32Z

This PR has one failing unit test and needs to be rebased.

talldan · 2018-10-17T10:28:38Z

@jasmussen Cool - I've pushed a commit (3d08150) that does that. It should be easily reverted if there's a change of opinion.

youknowriad · 2018-10-17T11:42:48Z

So this means there's no "icon" in mobile which was an explicit decision because the "Save Draft" label might be long in some languages. I just want to make sure that we're doing this on purpose :)

jasmussen · 2018-10-17T11:50:54Z

Oh I'm sorry the save icon should be there on mobile and only mobile, like before

youknowriad · 2018-10-18T07:51:37Z

packages/components/src/toggle-control/index.js

@@ -40,7 +40,7 @@ class ToggleControl extends Component {

 		return (
 			<BaseControl
-				label={ label }
+				label={ heading }


I'm not sure I understand why we need the changes in this component? cc @jasmussen ?

If my assumption is correct you wanted to align the label at the right so you duplicated it and passed something inexisting to the parent component? I tweaked the component and use order CSS instead.

Are we sure the order property is kosher, tab direction wise?

I think it is, since the label is not a tab stop

youknowriad · 2018-10-18T07:55:54Z

packages/components/src/form-toggle/style.scss

@@ -4,8 +4,9 @@ $toggle-border-width: 2px;

 .components-form-toggle {
 	position: relative;
+	margin-right: $grid-size-large;


It seems weird to add a margin right globally too all toggles? Why do we need this?

I moved it to the ToggleControl style as it seems that this was specific to that component instead.

The thing is, the label is clickable. Usually, the bigger the clickable area, the easier it is to hit and therefore the more user-friendly it is. Because this is a flex container, the setting fixed margin can stretch the element. This was why.

I can't test if the new rules achieve the same, and if they don't that's not the end of the world. Thanks for fixing. ♥️

Don't worry, I applied the exact same style, I just moved it elsewhere to keep the genericity of the form toggle component.

Not sure how this affects the label's clickable area though.

It's fine, it's not visible, so it's an enhancement that can happen later.

Did the necessary updates.

youknowriad

LGTM 👍

youknowriad · 2018-10-18T08:21:40Z

Actually, I'm seeing broken styling for the "trash" button. Hold on merging, I'm looking.

Edit: It's fine, issue with my browser's cache.

jasmussen · 2018-10-18T09:08:17Z

🌈

One milkshake for Mr. Benguella!

🥤

youknowriad · 2018-10-18T09:09:57Z

And another for @talldan :)

jasmussen · 2018-10-18T09:13:51Z

Another one for Mr. Richards, yes!

🥤

afercia · 2018-10-21T13:46:31Z

I've noticed a couple things worth considering as quick improvements.

1
The "Switch to Draft" button clickable area is maybe a bit too small and it's different from the "Save Draft" button. Both buttons have a .is-tertiary class but then the "Save Draft" 8 pixels padding is defined separately in packages/editor/src/components/post-saved-state/style.scss. Instead, the "Switch to Draft" button doesn't have this padding.

Will leave this to the better knowledge of @jasmussen

2

Thirdly, it moves the switch to the left of the label:

This change allows now to address a long standing issue with the toggle control. Under the hood, it uses a <checkbox> element and a label. Checkboxes should always come first and be followed by their labels in the DOM order. Changing the visual order with the flexbox order property is not ideal because visual order and DOM order don't match (see WCAG), see also https://www.w3.org/TR/css-flexbox-1/#order-accessibility

Will prepare a quick PR.

This addressed feedback in #10552 (comment). It makes the tertiary button style be born with the same hit area as a button, because it is intended to be used as such, just with a different styling.

* Fix issue with tertiary button hit areas This addressed feedback in #10552 (comment). It makes the tertiary button style be born with the same hit area as a button, because it is intended to be used as such, just with a different styling. * Address feedback. * Clarify comment.

* Polish Save Draft/Switch to Draft area * Use a Button for Trash button * Polish the toggles.

* Fix issue with tertiary button hit areas This addressed feedback in WordPress#10552 (comment). It makes the tertiary button style be born with the same hit area as a button, because it is intended to be used as such, just with a different styling. * Address feedback. * Clarify comment.

jasmussen added [Type] Enhancement A suggestion for improvement. Needs Design Feedback Needs general design feedback. labels Oct 12, 2018

jasmussen added this to the 4.1 milestone Oct 12, 2018

jasmussen self-assigned this Oct 12, 2018

jasmussen requested a review from a team October 12, 2018 12:49

jasmussen requested a review from karmatosed October 13, 2018 14:32

This was referenced Oct 14, 2018

Animated Autosaving icon is very distracting #10415

Closed

Cloud Icons as saving status in Gutenberg #9455

Closed

karmatosed approved these changes Oct 15, 2018

View reviewed changes

karmatosed removed the Needs Design Feedback Needs general design feedback. label Oct 15, 2018

joshuatf mentioned this pull request Oct 16, 2018

Tabular Data: Update download button to match design documentation woocommerce/woocommerce-admin#442

Closed

Rename is-text button style to is-tertiary

3d08150

talldan force-pushed the polish/polish branch from a1d4fd5 to 3d08150 Compare October 17, 2018 10:29

Restore the save icon button for mobile

0fa20b3

youknowriad reviewed Oct 18, 2018

View reviewed changes

Tweak the toggles to reuse the label from the FormBase component

a06ffdc

Remove debug code

21a0e32

youknowriad approved these changes Oct 18, 2018

View reviewed changes

Fix unit tests

0071740

youknowriad merged commit a0e3ee5 into master Oct 18, 2018

youknowriad deleted the polish/polish branch October 18, 2018 09:02

This was referenced Oct 21, 2018

Toggle control: visual order should match DOM order #10868

Closed

Improve the Toggle Control DOM order for better accessibility #10870

Merged

jasmussen mentioned this pull request Oct 22, 2018

Fix issue with tertiary button hit areas #10888

Merged

antpb pushed a commit to antpb/gutenberg that referenced this pull request Oct 26, 2018

Polish tertiary actions, switches, save state (WordPress#10552)

12deed2

* Polish Save Draft/Switch to Draft area * Use a Button for Trash button * Polish the toggles.

kjellr mentioned this pull request May 8, 2019

Update save indicator contrast to pass AA #15514

Merged

epiqueras mentioned this pull request Sep 20, 2019

Editor: Fix and align collapsing logic for "Save Draft" and "Saved" button states. #17506

Merged

5 tasks

Polish tertiary actions, switches, save state #10552

Polish tertiary actions, switches, save state #10552

Conversation

jasmussen commented Oct 12, 2018

youknowriad commented Oct 12, 2018

youknowriad commented Oct 12, 2018

jasmussen commented Oct 12, 2018 • edited Loading

mtias commented Oct 12, 2018

aduth commented Oct 12, 2018

alexislloyd commented Oct 12, 2018

jasmussen commented Oct 12, 2018

jasmussen commented Oct 12, 2018

aduth commented Oct 12, 2018

aduth commented Oct 12, 2018 • edited Loading

mtias commented Oct 12, 2018

aduth commented Oct 12, 2018 • edited Loading

mtias commented Oct 12, 2018

jasmussen commented Oct 12, 2018

ZebulanStanphill commented Oct 12, 2018

youknowriad commented Oct 12, 2018

ZebulanStanphill commented Oct 12, 2018

robertdall commented Oct 13, 2018 • edited Loading

karmatosed left a comment

Choose a reason for hiding this comment

gziolo commented Oct 16, 2018

talldan commented Oct 17, 2018 • edited Loading

youknowriad commented Oct 17, 2018

jasmussen commented Oct 17, 2018

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

youknowriad left a comment

Choose a reason for hiding this comment

youknowriad commented Oct 18, 2018 • edited Loading

jasmussen commented Oct 18, 2018

🥤

youknowriad commented Oct 18, 2018

jasmussen commented Oct 18, 2018

🥤

afercia commented Oct 21, 2018

jasmussen commented Oct 12, 2018 •

edited

Loading

aduth commented Oct 12, 2018 •

edited

Loading

aduth commented Oct 12, 2018 •

edited

Loading

robertdall commented Oct 13, 2018 •

edited

Loading

talldan commented Oct 17, 2018 •

edited

Loading

youknowriad commented Oct 18, 2018 •

edited

Loading