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.

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 wide image jumping. #12305

Merged
merged 1 commit into from
Nov 26, 2018
Merged

Fix wide image jumping. #12305

merged 1 commit into from
Nov 26, 2018

Conversation

jasmussen
Copy link
Contributor

@jasmussen jasmussen commented Nov 26, 2018

Fixes #12292.

This PR fixes an issue where the block toolbar would cause an image to jump downwards when the wide or fullwide buttons were pressed.

Recently as part of a floats refactor, we also refactored how the block toolbar worked. This meant the removal of a floats rule to the toolbar itself, because it was both unnecessary and interfered with adjacent floats. This PR restores that rule, but for wide and fullwide only, fixing the regression.

fullwid

I also fixed a small math issue with the toolbar-width on wide images.

It's a very small regression, and this doesn't need to be in 5.0.

But I'm adding it to the 4.6 milestone regardless, as a means of offering it up for inclusion review. Feel free to put in another milestone.

Fixes #12292.

This PR fixes an issue where the block toolbar would cause an image to jump downwards when the wide or fullwide buttons were pressed.

Recently as part of a floats refactor, we also refactored how the block toolbar worked. This meant the removal of a floats rule to the toolbar itself, because it was both unnecessary and interfered with adjacent floats. This PR restores that rule, but for wide and fullwide only, fixing the regression.
@jasmussen jasmussen added the [Type] Regression Related to a regression in the latest release label Nov 26, 2018
@jasmussen jasmussen added this to the 4.6 milestone Nov 26, 2018
@jasmussen jasmussen self-assigned this Nov 26, 2018
@jasmussen jasmussen requested a review from a team November 26, 2018 09:53
// 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.

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@youknowriad youknowriad merged commit 97d1171 into master Nov 26, 2018
@youknowriad youknowriad deleted the fix/fullwide-image-jump branch November 26, 2018 10:49
youknowriad pushed a commit that referenced this pull request Nov 29, 2018
Fixes #12292.

This PR fixes an issue where the block toolbar would cause an image to jump downwards when the wide or fullwide buttons were pressed.

Recently as part of a floats refactor, we also refactored how the block toolbar worked. This meant the removal of a floats rule to the toolbar itself, because it was both unnecessary and interfered with adjacent floats. This PR restores that rule, but for wide and fullwide only, fixing the regression.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Regression Related to a regression in the latest release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clicking Wide or Full alignment makes the Image block jump
2 participants