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

Jump to bottom

Fix container within navbar on smallest breakpoint #21722

Merged
merged 6 commits into from Jan 21, 2017
Merged

Fix container within navbar on smallest breakpoint #21722

merged 6 commits into from Jan 21, 2017

Conversation

pvdlg
Copy link
Contributor

@pvdlg pvdlg commented Jan 14, 2017 •
edited by bardiharborow

Fixes #21605 and fixes #21590.

At the smallest breakpoint the container has no with set. In the case of a container within a navbar, due to margin-right: auto and margin-left: auto the container content is coalesced toward the center.
This issue happen only in Chrome (see http://jsfiddle.net/rLqtw01t/ in chrome on xs viewport with and in another browser).

This PR set margin-right: 0 and margin-left: 0 for container within navbar under xs viewport width to avoid the issue.

@browner12
Copy link
Contributor

i like this, and this bug (?) is open in some other issues as well. One thing I proposed was instead of setting the margins, set the width: 100%. I believe they both accomplish the same thing, and this is a little more descriptive of what it is trying to accomplish.

@pvdlg pvdlg changed the title Fix container with in navbar on smallest breakpoint Fix container within navbar on smallest breakpoint Jan 15, 2017
@pvdlg
Copy link
Contributor Author

pvdlg commented Jan 15, 2017

It does accomplish the same thing. But the problem come from here where we set margin-left: auto and margin-right: auto in regular container. So I think it's better to reset the problematic attribute in our situation :)

@pvdlg
Copy link
Contributor Author

pvdlg commented Jan 15, 2017 •
edited

Actually, revisiting this PR it seems it's related to a Chrome bug.
Check this JSFiddle in Chrome and in another brower: http://jsfiddle.net/rLqtw01t/

It works normally in Safari and Firefox not in Chrome.

So not really sure we should include this fix in the code base as it seems related to a browser bug...

@bardiharborow
Copy link
Member

@vanduynslagerp if you could write a reduced test case (i.e without Bootstrap), then I can organise to file a Chrome bug.

@pvdlg
Copy link
Contributor Author

pvdlg commented Jan 21, 2017 •
edited

Here you go: http://jsfiddle.net/Lgn9v9zt/
Actually I think it's a bug in Safari, as Firefox behave the same as Chrome.

The problem is then a flex container with margin-left: auto; margin-right: auto; within a container with display: flex; flex-direction: column; is display full width on Safari.
This is why the collapsed header appears to works on Safari.

Safari:
2017-01-20_22-23-37

Firefox:
2017-01-20_22-23-53

Chrome:
2017-01-20_22-24-04

Bootstrap works as expected in Safari but not in Chrome or Firefox. So I think the fix in this PR is still relevant.
It seems that a bug in Safari make Bootstrap behave as expected.

On Mac OS X 10.12.2 and I'm using:

  • Chrome 56.0.2924.59 beta (64-bit)
  • Safari 10.0.2 (12602.3.12.0.1)
  • Firefox 50.1.0

@mdo
Copy link
Member

mdo commented Jan 21, 2017

Ahhh, I understand the issue now.

Auto margins are basically like pushes or pulls on flex items. We use auto to center a default display: block container, but when it's within a display: flex parent, it becomes a flex item that's trying to push from the left and from the right, resulting in a centered element. In reality, I believe Safari has the bug and Chrome and Firefox are correct here.

@pvdlg
Copy link
Contributor Author

pvdlg commented Jan 21, 2017

Yes, I think the bug is in Safari too. So this PR (or similar) is still relevant.
We can still open a bug with Safari, even though it doesn't "really" impact Bootstrap (as it make something work that shouldn't)

@mdo mdo added this to the v4.0.0-beta milestone Jan 21, 2017
@mdo mdo merged commit 9cf2355 into twbs:v4-dev Jan 21, 2017
This was referenced Jan 21, 2017
Closed
Closed
@pvdlg pvdlg deleted the navbar-container-width-xs branch January 21, 2017 22:54
@josmaissan josmaissan mentioned this pull request Jan 26, 2017
Closed
rudionrails added a commit to joblocal/bootstrap that referenced this pull request Jan 30, 2017
@rudionrails
Merge remote-tracking branch 'bootstrap/v4-dev' into joblocal-bootstrap4
cccde09 
* bootstrap/v4-dev: (979 commits)
  Add aria-label to docs search field
  Remove random cursor: default from pill nav (twbs#21835)
  Allow button toolbars to wrap (twbs#21826)
  fixes twbs#21567
  grunt (fixes twbs#21819)
  Add align self to navbar brand (twbs#21626)
  Rename order utilities to intended class names (twbs#21739)
  Pixel perfect navbar toggler (twbs#21821)
  Update _custom.scss imports in other builds (twbs#21825)
  Allow flex-based navs to wrap like they used to (twbs#21824)
  Remove `cursor: pointer;` (twbs#21812)
  Fix body padding in Dashboard and Jumbotron examples.
  Move htmllint to npm script.
  Change to markdown (twbs#21815)
  Change header to use markdown (twbs#21809)
  Update grid layout docs (twbs#21806)
  grunt
  Drop Normalize, port relevant parts to Reboot (twbs#21741)
  grunt
  Fix container within navbar on smallest breakpoint (twbs#21722)
  ...
rudionrails added a commit to joblocal/bootstrap that referenced this pull request Jan 30, 2017
@rudionrails
Merge remote-tracking branch 'bootstrap/v4-dev' into joblocal-bootstrap4
e0e6c47 
* bootstrap/v4-dev: (979 commits)
  Add aria-label to docs search field
  Remove random cursor: default from pill nav (twbs#21835)
  Allow button toolbars to wrap (twbs#21826)
  fixes twbs#21567
  grunt (fixes twbs#21819)
  Add align self to navbar brand (twbs#21626)
  Rename order utilities to intended class names (twbs#21739)
  Pixel perfect navbar toggler (twbs#21821)
  Update _custom.scss imports in other builds (twbs#21825)
  Allow flex-based navs to wrap like they used to (twbs#21824)
  Remove `cursor: pointer;` (twbs#21812)
  Fix body padding in Dashboard and Jumbotron examples.
  Move htmllint to npm script.
  Change to markdown (twbs#21815)
  Change header to use markdown (twbs#21809)
  Update grid layout docs (twbs#21806)
  grunt
  Drop Normalize, port relevant parts to Reboot (twbs#21741)
  grunt
  Fix container within navbar on smallest breakpoint (twbs#21722)
  ...
This was referenced Feb 18, 2017
Closed
Closed
@mdo mdo mentioned this pull request Mar 15, 2017
Closed
This was referenced Mar 28, 2017
Closed
Closed
@istratos istratos mentioned this pull request Jul 26, 2017
Closed
@mdo mdo mentioned this pull request Aug 4, 2017
Merged
mdo added a commit that referenced this pull request Aug 4, 2017
@mdo
given width: 100% is always on a container now and it's constrained…
987ac63 
… by max-width, we can remove our navbar changes from #21722 and #22383
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
browser-bug css v4
Projects
None yet
Milestone
v4.0.0-beta
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@pvdlg @browner12 @bardiharborow @mdo