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

Changed last-child to first-child to fix #13325 #13464

Merged
merged 1 commit into from
Jul 6, 2014

Conversation

NickColley
Copy link
Contributor

Fixes #13325.
Fixes incorrect margin fix as the right most element will actually be the first in the DOM.
As noted by @jerem

@@ -327,7 +327,7 @@
.box-shadow(none);

// Outdent the form if last child to line up with content down the page
Copy link
Collaborator

Choose a reason for hiding this comment

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

Need to adjust this comment and the similar one below.

Choose a reason for hiding this comment

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

NAVER - http://www.naver.com/

lee36656@naver.com 님께 보내신 메일 <Re: [bootstrap] #13325 - Changed last-child to first-child (#13464)> 이 다음과 같은 이유로 전송 실패했습니다.


받는 사람이 회원님의 메일을 수신차단 하였습니다.


@cvrebert cvrebert changed the title #13325 - Changed last-child to first-child Changed last-child to first-child to fix #13325 Apr 30, 2014
As noted by @jerem the first child will actually be the last shown
@NickColley
Copy link
Contributor Author

Alright @cvrebert sorted that.

@loic
Copy link
Contributor

loic commented Apr 30, 2014

I don't think first-child is appropriate here. The navbar will certainly have other elements appearing first in the DOM like a left aligned navbar-var or navbar-form.

I believe first-of-type is the right selector here.

@NickColley
Copy link
Contributor Author

Since it's only being applied on navbar items being floated right is that a problem?

@mdo
Copy link
Member

mdo commented May 1, 2014

As far as I know, none of those solutions are going to work in every use case. This is just a larger problem with the limitation of the navbar given it's current structural constraints. Not sure there's a decent solution right now.

Can you test this with a bunch of navbar layouts in a jsbin?

@loic
Copy link
Contributor

loic commented May 1, 2014

POC: http://jsbin.com/zamolure/1/.

@mdo
Copy link
Member

mdo commented May 13, 2014

I screwed with this a big more to get to http://jsbin.com/zamolure/6/.

This should work in fixing things, but we'll need more testing. Punting to v3.2.1 for now so we can let this bake a bit more.

@mdo mdo added this to the v3.2.1 milestone May 13, 2014
@NickColley
Copy link
Contributor Author

Looking better, I noticed when those examples end up full width there's a gap exposed.
Check out this version to see the difference:
http://jsbin.com/zamolure/7/

I'll see if I can work up some more examples to test against.

@mdo
Copy link
Member

mdo commented Jun 11, 2014

@NickColley Did you end up doing any more testing on this? I haven't revisited since my last comment.

@loic
Copy link
Contributor

loic commented Jun 11, 2014

I've been using the fix I proposed in http://jsbin.com/zamolure/1/, it works well for me both in FF and Blink/Webkit, I don't have an IE to test it against.

I can't reproduce the gap mentioned by @NickColley.

@NickColley
Copy link
Contributor Author

@loic you need to have the example /6/ small enough that it goes into the full width mobile view. There's a gap at the end.

@loic
Copy link
Contributor

loic commented Jun 12, 2014

@NickColley right! The css should be wrapped in a media query: http://jsbin.com/zamolure/8/.

@mdo
Copy link
Member

mdo commented Jun 19, 2014

Downside to your latest @loic is that the margins between consecutive .navbar-right double up. See the space between "Link" and search input:
screen shot 2014-06-18 at 8 14 39 pm

@cvrebert
Copy link
Collaborator

Is nesting stuff within one navbar-right an option, as opposed to having multiple navbar-rights?

@loic
Copy link
Contributor

loic commented Jun 19, 2014

@mdo isn't that the expected spacing though? By my tests .navbar-left shows similar spacing.

@cvrebert just tried it, it doesn't seem to work.

@mdo
Copy link
Member

mdo commented Jun 19, 2014

@loic I'm not sure if one or the other should be expected or the intended styles, but what I am saying is that the spacing has changed between master and here. The consecutive .navbar-right spacing is doubled with these changes.

@loic
Copy link
Contributor

loic commented Jun 20, 2014

@mdo unless I'm misunderstanding the issue, this extra spacing is because .navbar-form { padding: 10px 15px; } can now express itself.

Just beefed up the example a little: http://jsbin.com/zamolure/12

We can see in the "Fixed" section that the spacing is now the same for right alignment as it has always been for left alignment.

I also added a "Without margins" section that prevents the double margin both on '.navbar-left' and '.navbar-right' but I don't think it's the expected behavior, especially on links.

@mdo
Copy link
Member

mdo commented Jul 6, 2014

Yeah I think this ends up fixing more things than breaking, so I'm good to merge it.

mdo added a commit that referenced this pull request Jul 6, 2014
@mdo mdo merged commit f9fde56 into twbs:master Jul 6, 2014
@mdo
Copy link
Member

mdo commented Jul 6, 2014

Thanks!

@cvrebert cvrebert mentioned this pull request Jul 6, 2014
@loic
Copy link
Contributor

loic commented Jul 6, 2014

@mdo you've merged the fix from @NickColley which is completely different from the fix I was proposing. AFAIK this commit doesn't address the issue and should be reverted.

I can make a new PR with the fix from my POC.

@mdo
Copy link
Member

mdo commented Jul 6, 2014

I don't think we'll revert as I think this still fixes a valid issue. But sure, open a new PR with more info and we'll revisit.

@loic
Copy link
Contributor

loic commented Jul 6, 2014

Well the fix is wrong in a sense that .navbar-right:first-child will only ever match anything if there is nothing left aligned in the navbar.

loic added a commit to loic/bootstrap that referenced this pull request Jul 7, 2014
loic added a commit to loic/bootstrap that referenced this pull request Jul 7, 2014
mdo added a commit that referenced this pull request Jul 7, 2014
Address margins on .navbar-right to fix #13325 #13464.
stempler pushed a commit to stempler/bootstrap that referenced this pull request Nov 4, 2014
…r-right-first

Changed last-child to first-child to fix twbs#13325
stempler pushed a commit to stempler/bootstrap that referenced this pull request Nov 4, 2014
stempler pushed a commit to stempler/bootstrap that referenced this pull request Nov 4, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Negative margin on '.navbar-right:last-child' in Navbar is a miscalculation
5 participants