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

Preview Button: Remove the separator and border, and reduce the size of the icon. #20683

Merged
merged 6 commits into from
Mar 13, 2020

Conversation

shaunandrews
Copy link
Contributor

There's a few visual things about the new device-preview button that have been bothering me.

The first is the separator between the currently selected device, and the dropdown arrow. The line makes me thing that this button does two things; I'd expect different behavior when clicking the label and when clicking the icon. But both the label and the icon both trigger the same action. So, I don't see the need for the line at all. This PR removes it.

The border around button feels a little strange. Not all buttons in the toolbar have a border, in fact I don't see any other buttons in the top toolbar, or block toolbars that use a border. So, I removed the border and kind of like how it feels. What do you think?

The icon's current size feels too heavy. When compared to other dropdown in the editor and WordPress, the icon is larger than others. So, I reduces the size of the icon down to 16px and it feels nicer. What do you think?

Current:
image

This PR:
image

This removes the separator and border from the preview toggle. It also reduces the size of the icon down to 16px. Along with this, some adjustments to the button padding were required.
@shaunandrews shaunandrews added the Needs Design Feedback Needs general design feedback. label Mar 6, 2020
@github-actions github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Mar 6, 2020
@github-actions
Copy link

github-actions bot commented Mar 6, 2020

Size Change: -7.59 kB (0%)

Total Size: 856 kB

Filename Size Change
build/annotations/index.js 3.43 kB -1 B
build/block-directory/index.js 6.03 kB +6 B (0%)
build/block-editor/index.js 100 kB -3.68 kB (3%)
build/block-editor/style-rtl.css 10.7 kB +154 B (1%)
build/block-editor/style.css 10.7 kB +149 B (1%)
build/block-library/editor-rtl.css 7.23 kB -122 B (1%)
build/block-library/editor.css 7.24 kB -121 B (1%)
build/block-library/index.js 111 kB -4.1 kB (3%)
build/block-library/style-rtl.css 7.38 kB -125 B (1%)
build/block-library/style.css 7.39 kB -122 B (1%)
build/components/index.js 191 kB -11 B (0%)
build/components/style-rtl.css 15.5 kB -11 B (0%)
build/components/style.css 15.5 kB -11 B (0%)
build/compose/index.js 6.22 kB +462 B (7%) 🔍
build/data-controls/index.js 1.04 kB +1 B
build/data/index.js 8.21 kB -3 B (0%)
build/date/index.js 5.37 kB +4 B (0%)
build/dom-ready/index.js 568 B -1 B
build/edit-post/style-rtl.css 8.62 kB -16 B (0%)
build/edit-post/style.css 8.62 kB -20 B (0%)
build/edit-site/style-rtl.css 2.48 kB -1 B
build/edit-site/style.css 2.48 kB -1 B
build/edit-widgets/index.js 4.45 kB +6 B (0%)
build/edit-widgets/style-rtl.css 2.58 kB +1 B
build/edit-widgets/style.css 2.58 kB +1 B
build/editor/index.js 43.8 kB +11 B (0%)
build/editor/style-rtl.css 3.97 kB -16 B (0%)
build/editor/style.css 3.96 kB -16 B (0%)
build/element/index.js 4.45 kB -1 B
build/escape-html/index.js 733 B -1 B
build/format-library/index.js 7.09 kB -15 B (0%)
build/html-entities/index.js 621 B -1 B
build/i18n/index.js 3.49 kB +2 B (0%)
build/list-reusable-blocks/index.js 2.99 kB -1 B
build/primitives/index.js 1.49 kB -1 B
build/priority-queue/index.js 779 B -1 B
build/rich-text/index.js 14.3 kB -2 B (0%)
build/url/index.js 4.01 kB +13 B (0%)
build/warning/index.js 1.14 kB +1 B
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.01 kB 0 B
build/api-fetch/index.js 3.39 kB 0 B
build/autop/index.js 2.58 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 760 B 0 B
build/block-library/theme-rtl.css 669 B 0 B
build/block-library/theme.css 671 B 0 B
build/block-serialization-default-parser/index.js 1.65 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 57.7 kB 0 B
build/core-data/index.js 10.6 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom/index.js 3.06 kB 0 B
build/edit-post/index.js 91.3 kB 0 B
build/edit-site/index.js 4.64 kB 0 B
build/editor/editor-styles-rtl.css 381 B 0 B
build/editor/editor-styles.css 382 B 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 1.92 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.3 kB 0 B
build/keycodes/index.js 1.68 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 4.85 kB 0 B
build/notices/index.js 1.57 kB 0 B
build/nux/index.js 3.01 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.54 kB 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/server-side-render/index.js 2.55 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/viewport/index.js 1.61 kB 0 B
build/wordcount/index.js 1.18 kB 0 B

compressed-size-action

@karmatosed
Copy link
Member

I really like this as gives some visual clarity. It also stops me wondering if the sizes are different on the drop-down and publish button.

@mtias
Copy link
Member

mtias commented Mar 7, 2020

This looks good. I think we should always say "preview" there and not show the name of the current breakpoint.

@@ -22,6 +22,7 @@ import { external, check } from '@wordpress/icons';

const downArrow = (
<SVG
className="editor-post-preview__button-icon"
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem great

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Teach me your ways. Whats wrong with it? We could totally do it without the className, but let me know why its bad.

Copy link
Member

Choose a reason for hiding this comment

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

I mean this should be something that gets applied by default by using the right ingredients, without having to manually update the SVG.

@karmatosed karmatosed removed the Needs Design Feedback Needs general design feedback. label Mar 9, 2020
@shaunandrews
Copy link
Contributor Author

image

I removed the dynamic label, replacing it with a static "Preview" label.

image

I also removed the "View" label, repositioned the external icon, and updated the label from "Preview externally" to "Preview in new tab".

justify-content: flex-start;

svg {
margin-right: 8px;
Copy link
Contributor

Choose a reason for hiding this comment

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

Screenshot 2020-03-13 at 14 10 26

This caused a few issues, so let's discuss this separately. In the mean time I removed it.

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.

I see a lot of positive reactions on this change, and good discussion. The PR was lovely, and nearly there. I took the liberty of massaging the pixels ever so slightly, and updating the External Link icon. Here's what it looks like now:

i1

This feels like a good evolution of the Preview dropdown. Let's try it!

Copy link
Member

@mtias mtias left a comment

Choose a reason for hiding this comment

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

Let's get this in, thanks for the iterations.

@mtias mtias merged commit c5d792a into master Mar 13, 2020
@mtias mtias deleted the try/preview-options-minimal branch March 13, 2020 16:26
@github-actions github-actions bot added this to the Gutenberg 7.8 milestone Mar 13, 2020
@shaunandrews
Copy link
Contributor Author

Thanks for taking this one to the finish line while I was AFK!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants