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 regression #10228

Merged
merged 2 commits into from
Sep 28, 2018
Merged

Fix wide regression #10228

merged 2 commits into from
Sep 28, 2018

Conversation

jasmussen
Copy link
Contributor

This fixes a regression where wide images would cause horizontal scrollbars. Try the demo content in master, note that there are horizontal scrollbars. Then try this branch, note that they are gone.

The issue: the margins on the figure weren't being reset left and right on wide image.

I will investigate why this regression happened, I suspect it was related to the side-padding normalization.

This fixes a regression where wide images would cause horizontal scrollbars. Try the demo content, for example.

I will investigate why this regression happened, I suspect it was related to the side-padding normalization.
@jasmussen jasmussen added the [Type] Bug An existing feature does not function as intended label Sep 28, 2018
@jasmussen jasmussen added this to the 4.0 milestone Sep 28, 2018
@jasmussen jasmussen self-assigned this Sep 28, 2018
@jasmussen jasmussen requested a review from a team September 28, 2018 08:51
Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

Things looked great in my tests 👍 I'm curious about the reason of this regression and if we can fix the problem by removing/changing the rule that caused this problem instead of adding the additional rules. But given that this bug is very noticeable feel free to merge and we can iterate later if we find a better approach.

@@ -361,6 +361,12 @@
display: inline-flex;
}
}

// This resets the intrinsic margin on the figure in wide and full-wide alignments.
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to apply this rules to the wide alignment? It looks like it was working as expected before and the problem only affected full alignment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't think we did, and of course the wide alignment won't cause a scrollbar. But it's also inaccurately represented if we don't also reset the margins there.

Oh golly, looks like it's needed even on non wide images:

screen shot 2018-09-28 at 13 27 56

One sec pushing fix.

@jasmussen
Copy link
Contributor Author

Thank you Jorge for the review, but I discovered this issue was bigger than just for wide and fullwide:

screen shot 2018-09-28 at 13 27 56

it's even for normal images that are inserted.

We used to simply set margin: 0; on the figure, but this caused issues with captions and spacing between blocks. Now we only reset the left and right margins.

Requesting another review because it's a biggish change. @kjellr can you look also?

Copy link
Contributor

@kjellr kjellr left a comment

Choose a reason for hiding this comment

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

This is working on my end. I'm also a little puzzled as to why this regression happened but these updated styles do fix it. I didn't find any new errors on mobile/various browsers.

Before

before

After

fixed

@jasmussen jasmussen merged commit dff799b into master Sep 28, 2018
@jasmussen jasmussen deleted the fix/wide-regression branch September 28, 2018 13:19
jasmussen added a commit that referenced this pull request Oct 3, 2019
Fixes #11234. Alternative to #10228.

This PR fixes an issue where styles meant for the editor bled into the theme.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants