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

Fix wide image jumping. #12305

Merged
merged 1 commit into from
Nov 26, 2018
Merged

Fix wide image jumping. #12305

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 8 additions & 4 deletions packages/edit-post/src/components/visual-editor/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,17 @@
height: 0; // This collapses the container to an invisible element without margin.
text-align: center;

// This float rule takes the toolbar out of the flow, without it having to be absolute positioned.
// This is necessary because otherwise the mere presence of the toolbar can push down content.
// Pairs with relative rule on line 49.
float: left;
Copy link
Contributor

Choose a reason for hiding this comment

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

Didn't we remove this for another reason? Just want to make sure we're not regressing something else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We removed it because it was one way to take the toolbar out of the flow of the post. But that method for taking it out of the flow became redundant with other refactors to the block toolbar, and so we took the rule out partially because it was redundant, and partially to improve how float adjacent blocks handled.

For example you might have a left floated image with a paragraph after it. With the float rule on the toolbar intact, you'd get this:

screenshot 2018-11-26 at 11 00 13

By removing the float rule, and instead applying a position: absolute (the one we override in that code above), we get this:

floats

So will this regress other things? No, it shouldn't, because it adds only a single rule that's scoped to wide and fullwide blocks, and since those types of blocks should always clear floats that preceed them, there shouldn't be situations where this float would mis-position the toolbar.

If we wanted to test, the test case would be to have a float followed by a wide image, or a float followed by a fullwide image, and then testing that the block toolbar looks right in both sitautions. Which in my testing it does.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the detailed explanation, that makes sense to me 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 👍

It's a good question to be asked, an important one for deciding if/when to merge.


.editor-block-toolbar {
max-width: $content-width;
width: 100%;

// Necessary for the toolbar to be centered.
// This unsets an absolute position that will otherwise left align the toolbar.
position: relative;
}
}
Expand All @@ -47,10 +55,6 @@
width: 100%;
margin-left: 0;
margin-right: 0;

.editor-block-toolbar {
max-width: $content-width - $border-width - $border-width;
}
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion packages/editor/src/components/block-list/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -870,7 +870,7 @@
// Hide right border on desktop, where the .components-toolbar instead has a right border.
border-right: none;

// This prevents floats from messing up the position.
// This prevents floats from messing up the position of the block toolbar on floats-adjacent blocks when selected.
position: absolute;
left: 0;
}
Expand Down