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

Fix the balance of the WordPress logo and inserter. #21811

Merged
merged 1 commit into from
Apr 24, 2020

Conversation

jasmussen
Copy link
Contributor

These two regressed recently. The margin between the WordPress logo and the inserter button has been balanced to optically match that of the more menu on the right. The Inserter button was set to 32x32 to not be too imposing in the top toolbar, this was unintentionally removed.

Before:

before menu

After:

after

These two regressed recently. The margin between the WordPress logo and the inserter button has been balanced to optically match that of the more menu on the right. The Inserter button was set to 32x32 to not be too imposing in the top toolbar, this was unintentionally removed.
@jasmussen jasmussen self-assigned this Apr 23, 2020
@github-actions
Copy link

Size Change: -6 B (0%)

Total Size: 845 kB

Filename Size Change
build/edit-post/style-rtl.css 12.5 kB +4 B (0%)
build/edit-post/style.css 12.5 kB +5 B (0%)
build/edit-site/style-rtl.css 5.24 kB -2 B (0%)
build/edit-site/style.css 5.24 kB -6 B (0%)
build/edit-widgets/style-rtl.css 5 kB -3 B (0%)
build/edit-widgets/style.css 5 kB -4 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.02 kB 0 B
build/annotations/index.js 3.62 kB 0 B
build/api-fetch/index.js 4.08 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 6.24 kB 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 761 B 0 B
build/block-editor/index.js 105 kB 0 B
build/block-editor/style-rtl.css 10.1 kB 0 B
build/block-editor/style.css 10.1 kB 0 B
build/block-library/editor-rtl.css 7.13 kB 0 B
build/block-library/editor.css 7.13 kB 0 B
build/block-library/index.js 112 kB 0 B
build/block-library/style-rtl.css 7.19 kB 0 B
build/block-library/style.css 7.19 kB 0 B
build/block-library/theme-rtl.css 683 B 0 B
build/block-library/theme.css 685 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 57.7 kB 0 B
build/components/index.js 198 kB 0 B
build/components/style-rtl.css 16.9 kB 0 B
build/components/style.css 16.9 kB 0 B
build/compose/index.js 6.66 kB 0 B
build/core-data/index.js 11.4 kB 0 B
build/data-controls/index.js 1.25 kB 0 B
build/data/index.js 8.43 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.1 kB 0 B
build/edit-navigation/index.js 3.54 kB 0 B
build/edit-navigation/style-rtl.css 485 B 0 B
build/edit-navigation/style.css 485 B 0 B
build/edit-post/index.js 28.2 kB 0 B
build/edit-site/index.js 10.8 kB 0 B
build/edit-widgets/index.js 8.33 kB 0 B
build/editor/editor-styles-rtl.css 428 B 0 B
build/editor/editor-styles.css 431 B 0 B
build/editor/index.js 43.3 kB 0 B
build/editor/style-rtl.css 3.27 kB 0 B
build/editor/style.css 3.27 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.32 kB 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 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 711 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.12 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 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 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.67 kB 0 B
build/primitives/index.js 1.49 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/rich-text/index.js 14.8 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.28 kB 0 B
build/url/index.js 4.02 kB 0 B
build/viewport/index.js 1.84 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@@ -10,7 +10,6 @@
border: none;
border-radius: 0;
height: auto;
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.

Without this the logo and the Inserter seem too far apart.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I disagree quite a bit, and find that spacing to be a key ingredient of the more spacy nature of the G2 design. It's part of the 12px grid, and it's balanced towards the space on the right:

Screenshot 2020-04-23 at 15 03 17

You're not the only one to disagree with this, which suggests it's worth having a conversation about: do we make exceptions to the 12px grid, or is there a way to re-balance the blue button? But as a concerted G2 effort to create a holistic design, it's a little hard to keep up with small changes bundled in with others.

If we decide to revisit, the change in spacing should happen on the right element, as it is, this pulls the header area towards it:

Screenshot 2020-04-23 at 15 02 46

Copy link
Contributor

Choose a reason for hiding this comment

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

Here's what I see when I look at your suggested spacing:

image

All the green spaces are the same size, but then there's that random looking red space.

What I'd expect is for the space around the + button to be the same on all sides.

image

Copy link
Contributor Author

@jasmussen jasmussen Apr 24, 2020

Choose a reason for hiding this comment

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

This is the grid that G2 was designed towards:

Screenshot 2020-04-24 at 10 39 57

That toggled state is 32px wide, which is optically balanced to be between the 24px button and the outer edge of the block toolbar. If the black square was 36px, then sequential icons, bold italic and so on, would but up against each other.

It's an ongoing effort to make everything adhere properly to the grid, and actually that 32px size is one of the things that I promised Riad that we should look into. As he noted, most buttons are 36px in dimensions, which feels too big for the inserter and certainly is too big for toggle buttons in the block toolbar. It also makes the focus style for the inserter inconsistent with the other buttons. So there's definitely an inconsistency here that I think we should address holistically.

In the mean time, though, I would consider this PR less of a design discussion and more of a fix for a regression given the discussion to add a left padding to match the G2 mockups here: #20685 (comment)

Would you mind if we merged this fix and then opened a separate ticket to discuss this margin? We could make it a ticket about how to apply that grid to the top toolbar, dimensions of the buttons, spacing between, all of that, and then address it all in one fell swoop!

Copy link
Contributor

Choose a reason for hiding this comment

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

Don’t let my complaining stop you from merging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, I created #21870 as a good and visible place to discuss this!

Copy link
Contributor

Choose a reason for hiding this comment

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

I just think all buttons should have the same size no matter where they are 🤷‍♂️

@paaljoachim
Copy link
Contributor

paaljoachim commented Apr 23, 2020

It would be more consistent having an even spacing between the various icons also the W icon area. As the layout feels more balanced when icons have a balanced spacing between them.

Copy link
Contributor

@youknowriad youknowriad 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 revert to the previous behavior and continue discussion on the issue.

@jasmussen jasmussen merged commit 2f83264 into master Apr 24, 2020
@jasmussen jasmussen deleted the fix/32px-inserter branch April 24, 2020 14:22
@github-actions github-actions bot added this to the Gutenberg 8.0 milestone Apr 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants