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 issues with floats and the side UI on wide and fullwide #7223

Merged
merged 3 commits into from Jun 21, 2018

Conversation

Projects
None yet
2 participants
@jasmussen
Contributor

jasmussen commented Jun 8, 2018

There are a few issues in master:

  • Wide doesn't fit the side UI in between 600 and 1000ish breakpoints
  • Fullwide wasn't fullwide
  • The block toolbar on floated images pushed content downwards

Edit: I added a few more fixes:

  • Title block did not respect editor styles properly and lacked some responsive nuance
  • There were a few responsive issues with blocks on mobile, causing a horizontal scrollbar
  • The side plus on an empty paragraph is now better positioned

This PR fixes those. Screenshots:

screen shot 2018-06-08 at 11 27 24

screen shot 2018-06-08 at 11 27 30

floats

Test on mobile and desktop, frontend and backend, that wide and fullwide and normal images and blocks look fine and don't cause horizontal scrollbars. Also test that floats behave as they should.

@jasmussen jasmussen added the [Type] Bug label Jun 8, 2018

@jasmussen jasmussen self-assigned this Jun 8, 2018

@jasmussen jasmussen requested review from WordPress/gutenberg-core and gziolo Jun 8, 2018

@jasmussen jasmussen added this to the 3.1 milestone Jun 13, 2018

@jasmussen jasmussen referenced this pull request Jun 21, 2018

Merged

Fix IE11 formatting toolbar visibility #7413

4 of 4 tasks complete
@gziolo

I left a few comments where we can consider some improvements, not blockers. Let me test it in the browser.

@@ -69,22 +69,22 @@
// The base width of the title should match that of blocks even if it isn't a block
.editor-post-title {
@include break-small() {
padding-left: $block-side-ui-padding;
padding-right: $block-side-ui-padding;
padding-left: $block-side-ui-width + $block-padding + $block-side-ui-clearance + $block-side-ui-clearance;

This comment has been minimized.

@gziolo

gziolo Jun 21, 2018

Member

It deserves it's own variable, given the complexity and the number of times it is used. It probably will allow to simplify it to:

$block-sth = $block-side-ui-width + $block-padding + 2 * $block-side-ui-clearance;
content: none;
// Hide block outline when an image is floated.
.editor-block-list__block-edit {

This comment has been minimized.

@gziolo

gziolo Jun 21, 2018

Member

Empty line can be removed.

@include break-small() {
padding-left: $block-side-ui-padding;
padding-right: $block-side-ui-padding;
padding-left: $block-side-ui-width + $block-padding + $block-side-ui-clearance + $block-side-ui-clearance;

This comment has been minimized.

@gziolo

gziolo Jun 21, 2018

Member

The same formula as the one above.

@include break-small() {
margin-left: -$block-side-ui-padding + $border-width;
margin-right: -$block-side-ui-padding + $border-width;
margin-left: -$block-side-ui-width - $block-padding - $block-side-ui-clearance - $border-width;

This comment has been minimized.

@gziolo

gziolo Jun 21, 2018

Member

This one is slightly different comparing to the stuff above, is it intended to have $block-side-ui-clearance included only once?

@@ -74,7 +74,7 @@ $empty-paragraph-height: $text-editor-font-size * 4;
right: $item-spacing; // show on the right on mobile
@include break-small {
left: -$icon-button-size - $block-side-ui-clearance - $block-parent-side-ui-clearance;
left: -$block-side-ui-width - $block-padding - $block-side-ui-clearance;

This comment has been minimized.

@gziolo

gziolo Jun 21, 2018

Member

Maybe this should be the base for the variable I proposed above?

}
// Stack borders on mobile.
border-top: 1px solid transparent;

This comment has been minimized.

@gziolo

gziolo Jun 21, 2018

Member

What do you think about using the following:

border: 1px solid transparent;
border-width-left: 0;
border-width-right: 0;

@include break-small() {
    border-width: 1px;
}

Would it read better?

jasmussen added some commits Jun 8, 2018

Fix regressions with horizontal scrollbar for wide and fullwide
This also positions the toolbar a little bit more accurately when wide and fullwide.
@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Jun 21, 2018

Great feedback. I created a new variable for the block container padding, $block-container-side-padding. I do agree more cleanup is necessary, so I've set myself a task to do that. However if it's okay with you, I'd like to get this branch merged in first, then #7365, as it touches a lot of the same code and the rebase will be gnarly, and after that I'll do a run at cleaning up the math and variable names. What do you think?

@gziolo

This comment has been minimized.

Member

gziolo commented Jun 21, 2018

All good, let me test locally before I 👍 this PR.

@gziolo

This comment has been minimized.

Member

gziolo commented Jun 21, 2018

For the record, two issues I found which I shared already with @jasmussen in DMs:

  • icons are cut on the small screens for full width:
    screen shot 2018-06-21 at 10 50 17
  • I can't see movers on mobile medium devices (600-1000px range)
@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Jun 21, 2018

Great feedback. I pushed a fix to the mobile toolbar being cut on fullwide blocks:

screen shot 2018-06-21 at 11 12 47

I can't see movers on mobile medium devices (600-1000px range)

I think because this PR has been in the queue for a bit, a separate refactor changed the behavior for the side UI to behave the same. That is, it shows up above the block instead, and in the case of the movers, they are hidden until the > 1000 breakpoint.

The argument for unification of those two is that themes might have a different implementation of "wide", so we need to be a bit more flexible with the side UI there. Whether it's good to hide the movers between 600 and 1000 I think is fair to look at separately.

@gziolo

This comment has been minimized.

Member

gziolo commented Jun 21, 2018

I noticed that movers are hidden for all align types but center - which isn't as bad as it seems. Let's move on and iterate in another PR.

@gziolo

gziolo approved these changes Jun 21, 2018

LGTM, with the note about medium sizes and non-center aligned blocks which we agreed to fix separately.

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Jun 21, 2018

Yay, thank you! And yes, I have some improvements coming in a separate pr for floats.

@jasmussen jasmussen merged commit 2fcfeb3 into master Jun 21, 2018

2 checks passed

codecov/project 46.92% (+<.01%) compared to 5b2fcd4
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jasmussen jasmussen deleted the fix/float-side-ui-regressions branch Jun 21, 2018

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