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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from
Apr 24, 2020
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion packages/edit-post/src/components/header/header-toolbar/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,11 @@
}
}

.edit-post-header-toolbar__inserter-toggle {
.edit-post-header-toolbar > .edit-post-header-toolbar__inserter-toggle {
margin-right: $grid-unit-10;
// Special dimensions for this button.
min-width: 32px;
width: 32px;
height: 32px;
padding: 0;
}
1 change: 0 additions & 1 deletion packages/interface/src/components/admin-menu-toggle/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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 🤷‍♂️

width: $header-height;
background: #23282e; // WP-admin gray.
transition: all 0.12s ease-in-out;
Expand Down