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

Polish mobile toolbar #7278

Merged
merged 6 commits into from
Jun 13, 2018
Merged

Polish mobile toolbar #7278

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e3740df
Add right border when block toolbar scrolls.
Jun 12, 2018
83ae1ea
Remove transformations from Block More menu.
Jun 12, 2018
32ae608
Polish mobile toolbar.
Jun 12, 2018
805e6b8
Don't show sibling inserter on mobile.
Jun 12, 2018
0462666
Address feedback and fix typo.
Jun 13, 2018
47374b1
Address feedback on comments.
Jun 13, 2018
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
2 changes: 1 addition & 1 deletion edit-post/assets/stylesheets/_variables.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// Blocks
$parent-block-padding: 28px; // padding of top level blocks, should be larger than $block-padding, otherwise a user can't select the parent from a child
Expand Down
36 changes: 27 additions & 9 deletions editor/components/block-list/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -581,17 +581,27 @@
}
}

// Show side UI inline below the block on mobile
// Show side UI inline below the block on mobile.
.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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

margin-left: -$block-padding;
border-top: 1px solid $light-gray-500;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


@include break-small() {
display: none;
}

// Movers, inserter, trash & ellipsis
// Show a shadow below the selected block to imply separation.
box-shadow: $shadow-below-only;
@include break-mobile() {
box-shadow: none;
}

// Movers, inserter, trash & ellipsis.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

.editor-inserter {
position: relative;
left: auto;
Expand Down Expand Up @@ -659,7 +669,12 @@
}

.editor-block-list__insertion-point-inserter {
display: flex;
// Don't show on mobile.
display: none;
@include break-mobile() {
display: flex;
}

position: absolute;
top: 0;
bottom: auto;
Expand Down Expand Up @@ -753,18 +768,17 @@

// Position toolbar below the block on mobile
position: absolute;
bottom: -$block-toolbar-height + 1px;
//bottom: -$block-toolbar-height + 1px;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔥

bottom: $block-toolbar-height;
left: $block-padding;
right: $block-padding;
box-shadow: $shadow-below-only;

// Position the contextual toolbar above the block, add 1px to each to stack borders
@include break-mobile() {
position: sticky;
bottom: auto;
left: auto;
right: auto;
box-shadow: none;
margin-top: -$block-toolbar-height - 1px;
margin-bottom: $block-padding + 1px;
}
Expand Down Expand Up @@ -813,9 +827,13 @@
width: 100%;
background: $white;
border: 1px solid $light-gray-500;
border-right: none;

// this prevents floats from messing up the position
// Hide right border on desktop, where the .components-toolbar instead has a right border.
@include break-small() {
border-right: none;
}

// This prevents floats from messing up the position.
position: absolute;
left: 0;

Expand Down
2 changes: 0 additions & 2 deletions editor/components/block-settings-menu/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import './style.scss';
import BlockModeToggle from './block-mode-toggle';
import BlockRemoveButton from './block-remove-button';
import BlockDuplicateButton from './block-duplicate-button';
import BlockTransformations from './block-transformations';
import SharedBlockSettings from './shared-block-settings';
import UnknownConverter from './unknown-converter';
import _BlockSettingsMenuFirstItem from './block-settings-menu-first-item';
Expand Down Expand Up @@ -100,7 +99,6 @@ export class BlockSettingsMenu extends Component {
{ count === 1 && <UnknownConverter uid={ firstBlockUID } role="menuitem" /> }
<BlockDuplicateButton uids={ uids } rootUID={ rootUID } role="menuitem" />
{ count === 1 && <SharedBlockSettings uid={ firstBlockUID } onToggle={ onClose } itemsRole="menuitem" /> }
<BlockTransformations uids={ uids } onClick={ onClose } itemsRole="menuitem" />
</NavigableMenu>
) }
/>
Expand Down
5 changes: 2 additions & 3 deletions editor/components/block-toolbar/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,10 @@
flex-grow: 1;
width: 100%;
background: $white;
overflow: auto; // allow horizontal scrolling on mobile
overflow: auto; // Allow horizontal scrolling on mobile.
position: relative;

// allow overflow on desktop
// Allow overflow on desktop.
@include break-small() {
overflow: inherit;
}
Expand All @@ -18,7 +18,6 @@
border-right: 1px solid $light-gray-500;
}

// this should probably have its own class
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

> div {
display: flex;
}
Expand Down