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

Make Preview and Save Draft buttons use the same style #21192

Merged
merged 1 commit into from
Jun 24, 2020

Conversation

ZebulanStanphill
Copy link
Member

@ZebulanStanphill ZebulanStanphill commented Mar 27, 2020

Description

We were moving away from showing chevrons in buttons that open dropdown menus... so why does the Preview button still have one? This PR removes it.

One issue this raises to the surface is the inconsistency in design between the "Save Draft" and "Preview" buttons. I'm not really sure what an icon-less button should look like. What exactly is the standard?

UPDATE: this PR now changes both buttons to look the same and adjusts their padding to optically balance the spacing between them.

Screenshots

image
image

UPDATE: the buttons now look like this:
image
image

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@ZebulanStanphill ZebulanStanphill added the [Feature] UI Components Impacts or related to the UI component system label Mar 27, 2020
@github-actions
Copy link

github-actions bot commented Mar 27, 2020

Size Change: -22 B (0%)

Total Size: 1.13 MB

Filename Size Change
build/block-editor/index.js 107 kB -11 B (0%)
build/block-editor/style-rtl.css 10.6 kB -31 B (0%)
build/block-editor/style.css 10.6 kB -31 B (0%)
build/edit-post/style-rtl.css 5.5 kB +25 B (0%)
build/edit-post/style.css 5.5 kB +26 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.62 kB 0 B
build/api-fetch/index.js 3.4 kB 0 B
build/autop/index.js 2.82 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 7.27 kB 0 B
build/block-directory/style-rtl.css 937 B 0 B
build/block-directory/style.css 937 B 0 B
build/block-library/editor-rtl.css 7.6 kB 0 B
build/block-library/editor.css 7.6 kB 0 B
build/block-library/index.js 129 kB 0 B
build/block-library/style-rtl.css 8.03 kB 0 B
build/block-library/style.css 8.04 kB 0 B
build/block-library/theme-rtl.css 730 B 0 B
build/block-library/theme.css 732 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 48.2 kB 0 B
build/components/index.js 197 kB 0 B
build/components/style-rtl.css 15.8 kB 0 B
build/components/style.css 15.8 kB 0 B
build/compose/index.js 9.62 kB 0 B
build/core-data/index.js 11.4 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/data/index.js 8.44 kB 0 B
build/date/index.js 5.47 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 569 B 0 B
build/dom/index.js 3.19 kB 0 B
build/edit-navigation/index.js 9.87 kB 0 B
build/edit-navigation/style-rtl.css 1.02 kB 0 B
build/edit-navigation/style.css 1.02 kB 0 B
build/edit-post/index.js 303 kB 0 B
build/edit-site/index.js 16.6 kB 0 B
build/edit-site/style-rtl.css 3.02 kB 0 B
build/edit-site/style.css 3.02 kB 0 B
build/edit-widgets/index.js 9.33 kB 0 B
build/edit-widgets/style-rtl.css 2.42 kB 0 B
build/edit-widgets/style.css 2.42 kB 0 B
build/editor/editor-styles-rtl.css 537 B 0 B
build/editor/editor-styles.css 539 B 0 B
build/editor/index.js 44.9 kB 0 B
build/editor/style-rtl.css 3.81 kB 0 B
build/editor/style.css 3.81 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.72 kB 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.51 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.13 kB 0 B
build/list-reusable-blocks/style-rtl.css 450 B 0 B
build/list-reusable-blocks/style.css 451 B 0 B
build/media-utils/index.js 5.29 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 663 B 0 B
build/nux/style.css 660 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.5 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/rich-text/index.js 14 kB 0 B
build/server-side-render/index.js 2.67 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4.06 kB 0 B
build/viewport/index.js 1.85 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@ZebulanStanphill ZebulanStanphill marked this pull request as ready for review March 27, 2020 03:41
@ZebulanStanphill ZebulanStanphill added the Needs Design Feedback Needs general design feedback. label Mar 27, 2020
@ZebulanStanphill ZebulanStanphill force-pushed the update/preview-options-button-style branch from 62d09b9 to 15353cf Compare April 2, 2020 16:40
@ZebulanStanphill ZebulanStanphill added the [Type] Enhancement A suggestion for improvement. label Apr 3, 2020
@ZebulanStanphill ZebulanStanphill force-pushed the update/preview-options-button-style branch from 15353cf to 7c676f3 Compare May 5, 2020 16:14
@ZebulanStanphill ZebulanStanphill force-pushed the update/preview-options-button-style branch from 7c676f3 to bf084a5 Compare May 5, 2020 16:46
@ZebulanStanphill ZebulanStanphill force-pushed the update/preview-options-button-style branch from bf084a5 to 9cd58a6 Compare May 5, 2020 17:02
@jasmussen
Copy link
Contributor

Interesting! And thank you for the pull request.

A PR like this is in the camp of "we can try this and learn"! Trying it out in code is a much better way to progress, than talking endlessly on tickets. In that vein, I wouldn't oppse getting a feel for this.

However my instinct is that this control has more in common with the dropdown than it does with icon button menu toggles:

77938587-f54d7d80-7283-11ea-82ea-5ebab3b914ab

I know there's a spectrum of consistency there, but in my mind it's the same design principle as having a solid button component that can also be presented in outline or text-only form. More than ensuring consistency that buttons all look the same, we leverage our good sensibilities to decide which presentation works best given its context.

Yes, we can use that same principle to decide that in the top toolbar, that preview button does not need an icon. But to me it's more about evaluating what we feel works best there, than ensuring consistency.

@pablohoneyhoney @mtias any thoughts?

@ZebulanStanphill
Copy link
Member Author

@jasmussen Yeah, I made this PR since I figured it would be easier to try it out in practice than just discussing it in an issue.

I'd argue this button is less like a select dropdown than a lot of other things, e.g. the block alignment toolbar dropdown button. The preview options button includes not only a 3-choice radio menu, but also an action link to open something in another tab. That makes it a lot less like a select dropdown than, say, the Tools (edit/select) dropdown that's also in the top toolbar.

@mapk
Copy link
Contributor

mapk commented May 6, 2020

Great questions being raised here! The Preview option works exactly the same as the ellipses on the end. It opens a dropdown that shows some editor settings which change the view immediately when selected.

Screen Shot 2020-05-06 at 7 41 07 AM

There seems to be two major differences:

  1. The Preview is limited to one type of setting change.
  2. The Preview is in word form, not an icon.

So how do we navigate the UI implementation with those observations? Maybe because the focus of the Preview is limited to one type of selection, it should remain as a pseudo selectbox component (ie. include the dropdown arrow)?

@ZebulanStanphill
Copy link
Member Author

ZebulanStanphill commented May 6, 2020

@mapk

The Preview is limited to one type of setting change.

Actually, it also has the "Preview in external tab" link, though I suppose that's not really a "setting". But even ignoring that, it still wouldn't be the only dropdown in the top toolbar with a single type of setting change:

image

So then the only real distinguishing factor is that it is a text button, rather than an icon button.

@mapk
Copy link
Contributor

mapk commented May 6, 2020

Good point, @ZebulanStanphill. I noted here #20877 (comment), which is an attempt to add a navigation ability to the top toolbar, that including a dropdown arrow would help visually.

I wasn't thinking about this particular issue, but if the similarity is that these two items are text-based and not icons... that starts aligning nicely.

@ZebulanStanphill
Copy link
Member Author

Well, the site editor things ended up not using chevrons. Notably, the site editor navigation buttons use the same styles as the preview button in this PR, so it looks like the "Save Draft" button is the one that is inconsistent.

So, how should we move forward here?

@ZebulanStanphill ZebulanStanphill changed the title PreviewOptions: remove chevron from button. PreviewOptions: remove chevron from button Jun 2, 2020
@jasmussen
Copy link
Contributor

@ZebulanStanphill good observations. I think next step is a PR if you're up for it? No better way to decide.

@ZebulanStanphill
Copy link
Member Author

I could definitely make a PR to simplify the Save Draft button.

I just looked into it, though, and it turns out that button is actually not using custom styles... it's using tertiary button styles, specifically the is-tertiary CSS class. Personally, I think that's really weird, because for something that's supposed to be "tertiary", it has a much stronger hover state than the Preview button. 😕

Do you still think I should make a PR to change it to a standard button?

@jasmussen
Copy link
Contributor

Personally, I think that's really weird, because for something that's supposed to be "tertiary", it has a much stronger hover state than the Preview button. 😕

I'm increasingly unsure that the terms primary, secondary, and tertiary to define the visual appearance of the buttons is not really helpful, because those terms suggest a hierarchy that is not always appropriate for the design. For example in embeds, buttons go primary, tertiary, tertiary. I think it might make sense to consider them as solid, outlined, and text-only buttons.

But that is a separate point, and does not detract from yours, and it does seem like the next thing to try is to make both the save draft button, and the preview dropdown, be the same as they are in the FSE experience. It might even be an extension, or refocus, of this PR. Thanks!

@ZebulanStanphill ZebulanStanphill force-pushed the update/preview-options-button-style branch 2 times, most recently from cd41bfb to 3d2dc16 Compare June 4, 2020 16:15
@ZebulanStanphill
Copy link
Member Author

But that is a separate point, and does not detract from yours, and it does seem like the next thing to try is to make both the save draft button, and the preview dropdown, be the same as they are in the FSE experience.

The Save Draft button isn't in the FSE experience, and the Preview dropdown button looks the same as in the post/page editor.

But anyway, I've updated the PR to change both buttons to appear consistent with other text-only buttons like the ones in the FSE experience.
image

@ZebulanStanphill ZebulanStanphill changed the title PreviewOptions: remove chevron from button Make Preview and Save Draft buttons use the same style Jun 4, 2020
@jasmussen
Copy link
Contributor

Here's what I see:

Screenshot 2020-06-05 at 08 15 15

As an exercise in reduction, this seems very promising, insofar as it surfaces inconsistencies that were previously overlooked. For example it immediately stands out to me that the titlecase, "Save Draft" is inconsistent with what we're moving towards elsewhere. We might want to change that to "Save draft".

I also wonder if we should unify the padding around this text only button to match tertiary buttons. Compare:

Screenshot 2020-06-05 at 08 15 50

Screenshot 2020-06-05 at 08 15 55

The padding of tertiary buttons is 6px compared to 6px 12px for primary and secondary buttons, so that the optical spacing of the buttons when not focused is balanced.

Here's what these tweaks would look like:

Screenshot 2020-06-05 at 08 16 44

Here's focus:

padding

I think this could work.

@ZebulanStanphill
Copy link
Member Author

@jasmussen That sounded like a good idea to me, so I started implementing it.

However, while working on it, I noticed something odd: some instances of the <Button> component use the isSecondary prop, while others don't use it. It looks like you have to use the prop to get the is-secondary class applied to the resulting HTML.

It makes me wonder: why do some use the prop and some don't? Are some of them invalid? If the isSecondary prop represents a default button (or at least that's what I would have assumed), then why isn't it applied by default? Should I apply the isSecondary prop to the button versions of the PostSavedState component?

@ZebulanStanphill ZebulanStanphill force-pushed the update/preview-options-button-style branch from 3d2dc16 to 4119241 Compare June 5, 2020 19:14
@shaunandrews
Copy link
Contributor

This looks good.

I do wonder why the padding on the left and right of the save and preview buttons is smaller than the publish button? The publish button uses 12px of padding, while these buttons now use 6px.

Here's how it looks now in this PR:

6px-padding

Here's how it would look if you made it 12px to match the publish button:

12px-padding

--

I wouldn't let this hold up merging, but it wasn't clear to me if this was intentional. (Sorry if I missed a previous discussion around this decision.)

@ZebulanStanphill
Copy link
Member Author

@shaunandrews The padding was changed to optically balance the spacing between the buttons (similar to what is done to the buttons in the rich text formatting tools toolbar group) and match existing tertiary buttons that already do the same thing. @jasmussen first suggested it 4 days ago.

@jasmussen
Copy link
Contributor

Yep, the reduced padding for tertiary buttons is intentional, and you have to compare the resting state rather than the focused state, as that is what you will see most. It's also the case with the media placeholders:

Screenshot 2020-06-09 at 09 08 45

It becomes especially important when stacked:

Screenshot 2020-06-09 at 09 09 36

Even that isn't perfect, but it's better than the alternative of a 12px indented stacked button.

@shaunandrews
Copy link
Contributor

I tend to disagree. The smaller padding looks wrong to my eyes, especially with the example of the vertical setup state for the image block; with different padding the text doesn’t line up.

That said, I doubt anyone will really notice and this is such a small thing that it’s not worth discussing more now. Thanks for the insights.

@shaunandrews
Copy link
Contributor

I just noticed that when I create a new document, I see this disabled preview button:

image

@ZebulanStanphill
Copy link
Member Author

For better or worse, that's what the standard disabled state of tertiary buttons looks like. If we want to change it, we should probably do so in a separate PR.

@paaljoachim
Copy link
Contributor

Associated PR waiting to be reviewed:
Update: Post publish buttons and alignment.
#22390

@ZebulanStanphill ZebulanStanphill force-pushed the update/preview-options-button-style branch 7 times, most recently from 913b17b to 50a38aa Compare June 17, 2020 16:33
@youknowriad
Copy link
Contributor

What's blocking here? I feel this is a good one to land before 5.5

@ZebulanStanphill ZebulanStanphill force-pushed the update/preview-options-button-style branch from 50a38aa to 71bbf10 Compare June 22, 2020 15:40
@jasmussen
Copy link
Contributor

I had my checks pass today by updating snapshots. Normaly it's enough to update the unit test snapshots:

npm run test-unit -- --updateSnapshot

but for my last two PRS, I had to do

npm run test-e2e -- -u

that last one took forever, but fixed the issue I was having.

@ZebulanStanphill ZebulanStanphill force-pushed the update/preview-options-button-style branch from 71bbf10 to ce58781 Compare June 23, 2020 02:15
Copy link
Contributor

@jasmussen jasmussen left a comment

Choose a reason for hiding this comment

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

Before:

Screenshot 2020-06-23 at 08 49 18

Screenshot 2020-06-23 at 08 49 23

After:

after

Screenshot 2020-06-23 at 08 48 42

This is a small change, but it adds a lot of noise reduction, at what does not feel like it's at the expense of anything. It also adds consistency with label case, and colors with buttons in the block toolbar:

Screenshot 2020-06-23 at 08 49 03

I think we should get this in.

Co-authored-by: Joen Asmussen <asmussen@gmail.com>
@ZebulanStanphill ZebulanStanphill force-pushed the update/preview-options-button-style branch from ce58781 to f1244d9 Compare June 23, 2020 18:14
@ZebulanStanphill
Copy link
Member Author

I'll merge once I can get the tests to pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] UI Components Impacts or related to the UI component system [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants