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

Polish mobile toolbar #7278

Merged
merged 6 commits into from Jun 13, 2018

Conversation

@jasmussen
Contributor

jasmussen commented Jun 12, 2018

This is very much an incremental and iterative improvement to the mobile toolbar & experience. It is not drastic at all, and further improvement is definitely needed. But it is one step on a path. It does this a few things.

Firstly, it adds a right border to the block toolbar, when that scrolls. Before you could get this:

screen shot 2018-06-12 at 09 31 18

Now you get this:

screen shot 2018-06-12 at 09 41 44

The above is still not ideal by any means, but it's, again, an iterative improvement.

It also removes transforms from the Block's "More" menu. I think that fixes #6449.

It polishes the mobile toolbar, and rearranges it so formatting is before mover and ellipsis:

screen shot 2018-06-12 at 10 10 36

This can perhaps be improved further, but keeping in mind the limitations of mobile safari this is still in the realm of improvements we can do now.

Finally, it removes the sibling inserter on mobile, because this interfered with tapping buttons, and it isn't that useful anyway given that you can see a "Add block" button right in the bottom toolbar.

@jasmussen jasmussen self-assigned this Jun 12, 2018

@jasmussen jasmussen requested review from karmatosed and WordPress/gutenberg-core Jun 12, 2018

@tofumatt

I think there are a few cleanups needed here.

@@ -39,7 +39,7 @@ $block-controls-height: 36px;
$icon-button-size: 36px;
$icon-button-size-small: 24px;
$inserter-tabs-height: 36px;
$block-toolbar-height: 37px;
$block-toolbar-height: $block-controls-height + 1px;

This comment has been minimized.

@tofumatt

tofumatt Jun 12, 2018

Member

I get why this is but a comment to explain it would be great 😄

margin-top: $item-spacing;
margin-top: $item-spacing + $block-toolbar-height; // Make room for the formatting bar above.
margin-right: -$block-padding;
margin-bottom: -$block-padding - 1px; // Include border.

This comment has been minimized.

@tofumatt

tofumatt Jun 12, 2018

Member

If the border is the 1px value we should be storing $border = 1px; somewhere or when we update one we might forget to update the other. Also then it's just easier/automatic.

.editor-block-list__block-mobile-toolbar {
display: flex;
flex-direction: row;
margin-top: $item-spacing;
margin-top: $item-spacing + $block-toolbar-height; // Make room for the formatting bar above.

This comment has been minimized.

@tofumatt

tofumatt Jun 12, 2018

Member

This comment helps, but // Make room for the height of the block toolbar above. is even more explicit/helpful.

margin-right: -$block-padding;
margin-bottom: -$block-padding - 1px; // Include border.
margin-left: -$block-padding;
border-top: 1px solid $light-gray-500;

This comment has been minimized.

@tofumatt

tofumatt Jun 12, 2018

Member

Yeah, looks like this 1px could be a variable, that'd help with the // include border comment above.

box-shadow: none;
}
// Movers, inserter, trash & ellipsis.

This comment has been minimized.

@tofumatt

tofumatt Jun 12, 2018

Member

Don't we use Oxford commas in WordPress style? This should be // Movers, inserter, trash, and ellipsis.

@@ -753,18 +768,17 @@
// Position toolbar below the block on mobile
position: absolute;
bottom: -$block-toolbar-height + 1px;
//bottom: -$block-toolbar-height + 1px;

This comment has been minimized.

@tofumatt
@@ -18,7 +18,6 @@
border-right: 1px solid $light-gray-500;
}
// this should probably have its own class

This comment has been minimized.

@tofumatt

tofumatt Jun 12, 2018

Member

Is this actually no longer the case? I'd still agree with the comment, I don't like seeing selectors like > div; they seem sloppy.

This comment has been minimized.

@jasmussen

jasmussen Jun 12, 2018

Contributor

My thinking in removing this comment was perhaps fast and loose. I think it's still the case that it should be a class, but I this not happening in the near future, and instead of leaving a "note to self" might as well remove it. I remember trying to add the class, but not finding a clean way to do it.

This comment has been minimized.

@tofumatt

tofumatt Jun 12, 2018

Member

All of that sounds useful to know, and rather like the comment should be expanded upon rather than deleted 😄

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Jun 12, 2018

Solid review points. I'll address them all!

Address feedback and fix typo.
The typo was a actually committed in https://github.com/WordPress/gutenberg/pull/6408/files but this fixes it.
@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Jun 13, 2018

I think I caught all your points. Additionally I found and fixed a typo from an old PR. Care to take another look? Thank you!

@tofumatt tofumatt self-requested a review Jun 13, 2018

@tofumatt

This comment has been minimized.

Member

tofumatt commented Jun 13, 2018

Sure thing! In the future, you can request a re-review by selecting me for review again:

2018-06-13 10 10 21

Doing it is more of a pain because the UI isn't amazing, but it means it shows up in my pull request queue as a review request assigned to me, which is easier to find and means I won't miss it, unlike a comment 😄

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Jun 13, 2018

Doing it is more of a pain because the UI isn't amazing, but it means it shows up in my pull request queue as a review request assigned to me, which is easier to find and means I won't miss it, unlike a comment 😄

mindblown.webm

👍 👍

@tofumatt

Looks good to me; minor tweaks needed but after they're done I say it's good once the tests pass.

@@ -391,7 +390,7 @@
// Mover and settings above
> .editor-block-mover,
> .editor-block-settings-menu {
top: -$block-side-ui-width - 1px; // move upwards the height of the button +1px for border
top: -$block-side-ui-width - $border-width; // Move upwards the height of the button & border.

This comment has been minimized.

@tofumatt

tofumatt Jun 13, 2018

Member

This is phrased a bit oddly. How about:

top: -$block-side-ui-width - $border-width; // This moves the menu up by the height of the button + border.
// put toolbar snugly to side edges on mobile
margin-left: -$block-padding - 1px; // stack borders
margin-right: -$block-padding - 1px;
// Put toolbar snugly to side edges on mobile.

This comment has been minimized.

@tofumatt

tofumatt Jun 13, 2018

Member

"Snugly" is a cute turn of phrase but is a bit of a colloquialism. Maybe there's a better way to phrase this for less adapt English folk?

}
// except for wide elements, this causes a horizontal scrollbar
// ... Except for wide elements, this causes a horizontal scrollbar.

This comment has been minimized.

@tofumatt

tofumatt Jun 13, 2018

Member

Technically after an ellipsis you probably don't want a capital letter. That said there's no preceding ellipsis here so I'd just remove the ellipsis.

@jasmussen jasmussen merged commit 3e76aad into master Jun 13, 2018

2 checks passed

codecov/project 46.67% (-0.05%) compared to 3504ae7
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jasmussen jasmussen deleted the improve/mobile-toolbar branch Jun 13, 2018

@mtias mtias added this to the 3.1 milestone Jun 20, 2018

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