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(js): removes .modal .navbar-toggler margin #25745

Merged
merged 2 commits into from
Jul 11, 2018

Conversation

zalog
Copy link
Contributor

@zalog zalog commented Mar 1, 2018

The problem is that .navbar-toggler flips left when we open a bs modal dialog (gif or pen below).

I don't think we need to adjust .navbar-toggler in this case.
We'll always have a parent that manage to adjust the missing scroll width:

  • in case of a .fixed-* we have padding-right on that
  • in case of a no fixed navbar, we have padding-right on body

Here you have a pen and a demo:

demo

What do you think?

@ysds
Copy link
Member

ysds commented Mar 1, 2018

@zalog Thanks! (x-ref: #25417)

@Johann-S
Copy link
Member

Johann-S commented Mar 1, 2018

Please fix unit tests and it would be rad 👍

@zalog
Copy link
Contributor Author

zalog commented Mar 1, 2018

Yep, done. We're on green now.

@Johann-S
Copy link
Member

Johann-S commented Mar 1, 2018

Deleted our unit test, isn't a good solution, unless this case is already covered by a present unit test

@zalog
Copy link
Contributor Author

zalog commented Mar 1, 2018

I deleted the tests about $('<div class="navbar-toggler"></div>') because we don't need to test for margin-right. Now, after the first commit from this pr, we are not handling .navbar-toggler from js anymore.

Hmm... or is something that I'm missing? Sorry

@Johann-S
Copy link
Member

Johann-S commented Mar 1, 2018

But you made some changes, so we should test that

@zalog zalog force-pushed the zalog-modal-navbar-toggler branch 2 times, most recently from d7b674f to f407330 Compare March 6, 2018 16:29
@Johann-S Johann-S mentioned this pull request Mar 7, 2018
@zalog zalog force-pushed the zalog-modal-navbar-toggler branch from f407330 to 16c3f22 Compare March 12, 2018 13:31
@twbs-closer
Copy link

Hey there!

We're automatically closing this issue since the original poster (or another commenter) hasn't yet responded to the question or request made to them 14 days ago. We therefore assume that the user has lost interest or resolved the problem on their own. Closed issues that remain inactive for a long period may get automatically locked.

Don't worry though; if this is in error, let us know with a comment and we'll be happy to reopen the issue.

Thanks!

(Please note that this is an automated comment.)

@Johann-S Johann-S force-pushed the zalog-modal-navbar-toggler branch 2 times, most recently from 7d1398b to f91b716 Compare July 11, 2018 08:52
@Johann-S Johann-S merged commit 283ab30 into twbs:v4-dev Jul 11, 2018
@mdo mdo mentioned this pull request Jul 11, 2018
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.

5 participants