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

Beef up the padding on component panel titles. #1337

Merged
merged 3 commits into from
Jul 27, 2017
Merged

Beef up the padding on component panel titles. #1337

merged 3 commits into from
Jul 27, 2017

Conversation

JJJ
Copy link
Contributor

@JJJ JJJ commented Jun 21, 2017

This increases the size of the clickable target area for component panel titles by 30px in each direction, making them easier to click open & closed.

Fixes #1335.

This increases the size of the clickable target area for component panel titles by 30px in each direction, making them easier to click open & closed.

Fixes #1335.
@jasmussen
Copy link
Contributor

Nice! Thanks for doing this!

I think we should probably remove the border radius on the focus rectangle for this one:

screen shot 2017-06-21 at 17 09 09

With that done, though, 👍 👍

@aduth
Copy link
Member

aduth commented Jun 21, 2017

Alternative to negative margin is to create a separate wrapping component for the content itself, applying default padding there, and letting button / toggle render naturally at the bounds of the panel.

Downside is more nodes on the page.

Thoughts?

@aduth aduth added the General Interface Parts of the UI which don't fall neatly under other labels. label Jun 21, 2017
@JJJ
Copy link
Contributor Author

JJJ commented Jun 21, 2017

Thoughts?

I agree, negative margins are undesirable – it's hard for me to gauge to what degree for this project compared to others (WordPress itself, for example, is full of them.) My preference would be not to perpetuate the bad habit, but I didn't want to rough up Gutenberg too assertively out of the gate.

I think we should probably remove the border radius on the focus rectangle for this one

Good catch. I'll push that to my branch soon if no one else gets to it first.

@JJJ
Copy link
Contributor Author

JJJ commented Jun 23, 2017

I think we should probably remove the border radius

Once I removed the radius, it revealed there was a 1px margin between the button and the container elements. This had the undesirable effect of revealing a gap around the outline & box shadow, and because those container elements have horizontal borders already, I believe that gap was unintentional, so I've also set margin: 0; on those buttons in the same commit.

The result is a 2px reduction in overall button height in each panel (which could be mitigated with additional padding) and visually I think it still looks OK (if not better) but I don't want to be suggestive.

screen shot 2017-06-23 at 10 01 53 am

The missing right-outline is also undesirable, but fixing that is its own rabbit hole.

@youknowriad
Copy link
Contributor

Did you try to rebase here? I think the 1px margin might have been fixed.

@aduth
Copy link
Member

aduth commented Jul 27, 2017

Once I removed the radius, it revealed there was a 1px margin between the button and the container elements. This had the undesirable effect of revealing a gap around the outline & box shadow, and because those container elements have horizontal borders already, I believe that gap was unintentional, so I've also set margin: 0; on those buttons in the same commit.

Not sure if this one is browser-specific, but FWIW I was not able to reproduce this margin when removing the margin locally.

It feels a little crowded when focus highlight is active on an expanded panel, but I don't find it particularly problematic.

@aduth aduth merged commit 8b0c043 into WordPress:master Jul 27, 2017
@jasmussen
Copy link
Contributor

Thanks for this PR! Feels sooo much better now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
General Interface Parts of the UI which don't fall neatly under other labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Component panel titles are hard to click
4 participants