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 horizontal scrollbar when float element present #3635

Merged
merged 3 commits into from Nov 27, 2017

Conversation

Projects
None yet
3 participants
@Mathiu
Contributor

Mathiu commented Nov 23, 2017

Description

clear: left was only working for non aligned elements.

How Has This Been Tested?

Tested on PC Chrome 62 & Firefox 58 (beta5)

Checklist:

  • My code is tested. (partially i guess?)
  • My code follows the WordPress code style.
  • My code follows has proper inline documentation.
@@ -400,6 +406,7 @@
&:before {
content: '';
position: absolute;
left: 0;

This comment has been minimized.

@aduth

aduth Nov 27, 2017

Member

In my testing, this change alone is sufficient to resolve the issue without the addition of clear styles. Can you confirm?

@aduth

aduth Nov 27, 2017

Member

In my testing, this change alone is sufficient to resolve the issue without the addition of clear styles. Can you confirm?

This comment has been minimized.

@Mathiu

Mathiu Nov 27, 2017

Contributor

I think you're right, i remember that inserter wasn't wrapping correctly without the clear property, but I no longer can reproduce it and it actually wraps the text when there is aligned element and 2 separate paragraph elements after it.

Thanks for looking into it. I have removed unnecessary styles.

@Mathiu

Mathiu Nov 27, 2017

Contributor

I think you're right, i remember that inserter wasn't wrapping correctly without the clear property, but I no longer can reproduce it and it actually wraps the text when there is aligned element and 2 separate paragraph elements after it.

Thanks for looking into it. I have removed unnecessary styles.

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Nov 27, 2017

Member

Fixes #3612

Member

aduth commented Nov 27, 2017

Fixes #3612

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Nov 27, 2017

Codecov Report

Merging #3635 into master will increase coverage by 0.35%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #3635      +/-   ##
=========================================
+ Coverage   36.95%   37.3%   +0.35%     
=========================================
  Files         268     277       +9     
  Lines        6673    6690      +17     
  Branches     1202    1214      +12     
=========================================
+ Hits         2466    2496      +30     
+ Misses       3555    3536      -19     
- Partials      652     658       +6
Impacted Files Coverage Δ
blocks/library/paragraph/index.js 23.07% <0%> (-6.09%) ⬇️
editor/effects.js 58.06% <0%> (-2.11%) ⬇️
blocks/library/image/block.js 1.72% <0%> (-0.1%) ⬇️
editor/edit-post/modes/visual-editor/index.js 0% <0%> (ø) ⬆️
blocks/library/pullquote/index.js 37.5% <0%> (ø) ⬆️
editor/edit-post/header/header-toolbar/index.js 0% <0%> (ø) ⬆️
editor/edit-post/sidebar/post-trash/index.js 0% <0%> (ø) ⬆️
editor/edit-post/layout/index.js 0% <0%> (ø) ⬆️
blocks/editable/index.js 10.23% <0%> (ø) ⬆️
editor/components/block-list/index.js 0% <0%> (ø) ⬆️
... and 25 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2056a3c...96229d4. Read the comment docs.

codecov bot commented Nov 27, 2017

Codecov Report

Merging #3635 into master will increase coverage by 0.35%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #3635      +/-   ##
=========================================
+ Coverage   36.95%   37.3%   +0.35%     
=========================================
  Files         268     277       +9     
  Lines        6673    6690      +17     
  Branches     1202    1214      +12     
=========================================
+ Hits         2466    2496      +30     
+ Misses       3555    3536      -19     
- Partials      652     658       +6
Impacted Files Coverage Δ
blocks/library/paragraph/index.js 23.07% <0%> (-6.09%) ⬇️
editor/effects.js 58.06% <0%> (-2.11%) ⬇️
blocks/library/image/block.js 1.72% <0%> (-0.1%) ⬇️
editor/edit-post/modes/visual-editor/index.js 0% <0%> (ø) ⬆️
blocks/library/pullquote/index.js 37.5% <0%> (ø) ⬆️
editor/edit-post/header/header-toolbar/index.js 0% <0%> (ø) ⬆️
editor/edit-post/sidebar/post-trash/index.js 0% <0%> (ø) ⬆️
editor/edit-post/layout/index.js 0% <0%> (ø) ⬆️
blocks/editable/index.js 10.23% <0%> (ø) ⬆️
editor/components/block-list/index.js 0% <0%> (ø) ⬆️
... and 25 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2056a3c...96229d4. Read the comment docs.

@aduth

aduth approved these changes Nov 27, 2017

In my testing, there is some strange re-layout which occurs when first loading a post with floated content:

relayout

Since I can also reproduce on master, and noting that these changes address the issue of the horizontal scroll, I think it's safe to tackle separately.

@aduth aduth merged commit 1db24fc into WordPress:master Nov 27, 2017

2 checks passed

codecov/project 37.3% (+0.35%) compared to 2056a3c
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment