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

Remove additional side padding from blocks. #9653

Merged
merged 4 commits into from Sep 25, 2018

Conversation

Projects
None yet
5 participants
@jasmussen
Contributor

jasmussen commented Sep 6, 2018

You may have noticed that the padding left and right on a block is wider than above and below.

This was done in effort to make it simple to select the parent block by simply hovering it with the mouse. Arrow keys already traverse up to parent blocks and there's a clickthrough effect on mobile.

However in practice this additional padding has not felt sufficient, and was at the cost of some visual complexity. Instead, we'll be looking at improving parent child selection in #9628.

This PR thus reverts the style of this to how it used to be, with the same padding all around a block.

Reminder: this padding is purely visual — through negative margins, it doesn't actually affect the width of the block itself, or the margins above and below. See also #8350.

Before:

before

After:

after

Note that this PR touches a rather big CSS file that right now has a bunch of vestigial stuff related to the ellipsis menu on the right, as well as draggable areas left and right of the block. Notably as the incoming improvements to the drag handle are merged, this PR will probably need to be carefully rebased. CC: @nosolosw @youknowriad

@jasmussen jasmussen added this to the 3.9 milestone Sep 6, 2018

@jasmussen jasmussen self-assigned this Sep 6, 2018

@jasmussen jasmussen requested a review from mtias Sep 6, 2018

@jasmussen jasmussen requested a review from WordPress/gutenberg-core Sep 6, 2018

@chrisvanpatten

This comment has been minimized.

Show comment
Hide comment
@chrisvanpatten

chrisvanpatten Sep 6, 2018

Contributor

This makes me so happy

Contributor

chrisvanpatten commented Sep 6, 2018

This makes me so happy

@youknowriad youknowriad modified the milestones: 3.9, 4.0 Sep 14, 2018

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 20, 2018

Contributor

Rebased, tested. This seems solid.

Contributor

jasmussen commented Sep 20, 2018

Rebased, tested. This seems solid.

jasmussen added some commits Sep 6, 2018

Remove additional side padding from blocks.
You may have noticed that the padding left and right on a block is wider than above and below.

This was done in effort to make it simple to select the parent block by simply hovering it with the mouse. Arrow keys already traverse up to parent blocks and there's a clickthrough effect on mobile.

However in practice this additional padding has not felt sufficient, and was at the cost of some visual complexity. Instead, we'll be looking at improving parent child selection in #9628.

This PR thus reverts the style of this to how it used to be, with the same padding all around a block.

Reminder: this padding is purely visual — through negative margins, it doesn't actually affect the width of the block itself, or the margins above and below. See also #8350.
Remove references to ellipsis on the side.
This has been removed separately, this is just a cleanup job.
@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 25, 2018

Contributor

Tested yet again, rebased, and did a quick cleanup job to remove references to ellipsis on the side. Let's get this in.

On a sidenote, in testing this, I did a lot of testing on the columns block. This is sort of the notorious "it's hard to select the parent block" example. This remains the case. However, it is SO VERY VERY EASY to just use the arrow keys to select the parent. For example, select the column, then press up. This selects the parent. This is part of the cross-block arrow-key navigation, and it's been working for a while.

I'm not suggesting this arrow key navigation is sufficient, but I do think this speaks to the breadcrumbs and clickthrough as proposed in #9628 as being correct steps forward, as those features simply surface that feature more clearly.

Contributor

jasmussen commented Sep 25, 2018

Tested yet again, rebased, and did a quick cleanup job to remove references to ellipsis on the side. Let's get this in.

On a sidenote, in testing this, I did a lot of testing on the columns block. This is sort of the notorious "it's hard to select the parent block" example. This remains the case. However, it is SO VERY VERY EASY to just use the arrow keys to select the parent. For example, select the column, then press up. This selects the parent. This is part of the cross-block arrow-key navigation, and it's been working for a while.

I'm not suggesting this arrow key navigation is sufficient, but I do think this speaks to the breadcrumbs and clickthrough as proposed in #9628 as being correct steps forward, as those features simply surface that feature more clearly.

@talldan

This comment has been minimized.

Show comment
Hide comment
@talldan

talldan Sep 25, 2018

Contributor

Hey @jasmussen - I've been giving this a test this morning. It looks really good. I've found this one issue so far, the warning here is too high:

screen shot 2018-09-25 at 12 41 02 pm

Contributor

talldan commented Sep 25, 2018

Hey @jasmussen - I've been giving this a test this morning. It looks really good. I've found this one issue so far, the warning here is too high:

screen shot 2018-09-25 at 12 41 02 pm

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 25, 2018

Contributor

Excellent catch, thank you! That appears to have been the result of a bad rebase. In any case, it's fixed now.

In master:

screen shot 2018-09-25 at 14 03 19

This branch:

screen shot 2018-09-25 at 14 03 22

Contributor

jasmussen commented Sep 25, 2018

Excellent catch, thank you! That appears to have been the result of a bad rebase. In any case, it's fixed now.

In master:

screen shot 2018-09-25 at 14 03 19

This branch:

screen shot 2018-09-25 at 14 03 22

@talldan

This comment has been minimized.

Show comment
Hide comment
@talldan

talldan Sep 25, 2018

Contributor

@jasmussen One more tiny one 😄

I noticed this with the reusable block icon in the top right of the block and some blocks that have a solid background (images, embeds, etc.):
screen shot 2018-09-25 at 1 20 12 pm

Feels like maybe the icon should sit over the top of the image?

Contributor

talldan commented Sep 25, 2018

@jasmussen One more tiny one 😄

I noticed this with the reusable block icon in the top right of the block and some blocks that have a solid background (images, embeds, etc.):
screen shot 2018-09-25 at 1 20 12 pm

Feels like maybe the icon should sit over the top of the image?

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 25, 2018

Contributor

Great catch. Fixed it.

Note that I had to add a z index both to the indicator, and I had to elevate the z index of the hover breadcrumb as well. But I didn't see any negative side effects of doing this.

indicator

Contributor

jasmussen commented Sep 25, 2018

Great catch. Fixed it.

Note that I had to add a z index both to the indicator, and I had to elevate the z index of the hover breadcrumb as well. But I didn't see any negative side effects of doing this.

indicator

@talldan

This looks good to me. I've done a fair bit of testing in Chrome, IE11, and Edge. I could only spot the issues I mentioned.

It's nice that this includes quite a bit of removal of styles as well - yay! 🎉

Not sure if you also need a design approval as well, or maybe that's already happened in discussion elsewhere.

@mtias

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Sep 25, 2018

Contributor

This crosses off one of my biggest tiny annoyances, thanks!

Contributor

mtias commented Sep 25, 2018

This crosses off one of my biggest tiny annoyances, thanks!

@mtias mtias merged commit 6321a99 into master Sep 25, 2018

2 checks passed

codecov/project 48.83% (+0.01%) compared to 973d8c9
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@mtias mtias deleted the update/remove-parent-padding branch Sep 25, 2018

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 25, 2018

Contributor

😅🎉

Contributor

jasmussen commented Sep 25, 2018

😅🎉

@chrisvanpatten

This comment has been minimized.

Show comment
Hide comment
@chrisvanpatten

chrisvanpatten Sep 25, 2018

Contributor

Very excited to see all the red in this diff and happy to see this land!

Contributor

chrisvanpatten commented Sep 25, 2018

Very excited to see all the red in this diff and happy to see this land!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment