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(css): force .navbar .dropdown-menu positioning #22644

Merged
merged 2 commits into from
May 22, 2017

Conversation

zalog
Copy link
Contributor

@zalog zalog commented May 17, 2017

Demo

Problems:
Please follow my example in before, the problems are in the same order.

  • for navbars that never collapse .navbar.navbar-expand, .dropdown-menu has position: static and push parent .navbar container down
  • if .dropdown-menu it's on the edge of browser, popper will move it and seems unnatural for .navbar
  • if we use only .navbar (for navbar that always collapse), .dropdown-menu act in popper style

--

I tried to solve that by forcing .dropdown-menu to act like before popper, and in a mobile first abordation.

cc @Johann-S, #22628, #22630

@@ -160,7 +158,8 @@
flex-direction: row;

.dropdown-menu {
position: absolute;
position: absolute !important;
Copy link
Member

Choose a reason for hiding this comment

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

Not sure about that because Popper always use absolute position

Copy link
Contributor Author

@zalog zalog May 17, 2017

Choose a reason for hiding this comment

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

Yep, I know.
But now, because of mobile first abordation, we need to overwrite L80.

Why mobile first abordation:

  • for navbars that never collapse, .navbar.navbar-expand. My first navbar in the before example
  • for navbars that always collapse, just .navbar class. My third navbar example in that before pen

@Johann-S
Copy link
Member

Johann-S commented May 17, 2017

About that :

if .dropdown-menu it's on the edge of browser, popper will move it and seems unnatural for .navbar

If you user wants to disable that feature they can add data-flip="false" to disable that behavior

Sorry but I'm not sure about what you are trying to fix here

@zalog
Copy link
Contributor Author

zalog commented May 17, 2017

We can use data-flip but not for .navbar, I think. Here, .dropdown-menu must stay in place by default.
What do you think?

Preview:
image

--

This fix is for navbars that never collapse .navbar.navbar-expand and navbars that always collapse, just .navbar.

@FezVrasta
Copy link
Contributor

I'm not sure what are you trying to fix, the first codepen looks better than the second one...?

@zalog
Copy link
Contributor Author

zalog commented May 18, 2017

Before: http://codepen.io/zalog/pen/eWLvRP

  • first navbar example is for .navbar.navbar-expand, just click ok "Dropdown link" button please. It will push the .navbar down.
  • second example, just click on "Dropdown link". It will open like in my previous screenshot.
  • third navbar example is for .navbar without any complementar .navbar-expand or .navbar-expand-* classes. Click on the sandwitch button to collapse menu, and after that, click on the "Dropdown link"

@FezVrasta
Copy link
Contributor

I see the dropdowns broken only in the version when you add static !important and unset !important 😢

@zalog
Copy link
Contributor Author

zalog commented May 18, 2017

Yes, it's because of this commit.
The before example is with the current v4-dev branch.

@Johann-S
Copy link
Member

With your PR this issue will stay fixed : #22628 ?
And can you update your branch please

@zalog zalog force-pushed the fix-navbar-dropdown branch from bfbb443 to 1170ba5 Compare May 18, 2017 15:05
@zalog
Copy link
Contributor Author

zalog commented May 18, 2017

Yep, #22628 stays fixed.
Yes, I just rebased the branch.
Did you guys saw this bugs?

@Johann-S
Copy link
Member

I approve but it's a real problem to override Popper.js css with !important

@Johann-S Johann-S merged commit 3706c88 into twbs:v4-dev May 22, 2017
@Johann-S Johann-S added this to the v4.0.0-beta milestone May 22, 2017
@mdo mdo mentioned this pull request May 22, 2017
@zalog zalog deleted the fix-navbar-dropdown branch May 26, 2017 21:11
@zalog
Copy link
Contributor Author

zalog commented May 26, 2017

Yep I know... But this is the only way to override Popper.js and have .dropdown-menu controlled in the .navbar way.

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.

3 participants