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

Use css to display Dropdown in Navbar #22782

Merged
merged 3 commits into from
Jun 17, 2017
Merged

Use css to display Dropdown in Navbar #22782

merged 3 commits into from
Jun 17, 2017

Conversation

Johann-S
Copy link
Member

@Johann-S Johann-S commented Jun 8, 2017

In this PR, we remove CSS aplied by Popper.js (only for Dropdown in navbar) to let our CSS do the job.

We detect a weird behavior, If we have a navbar in 100% width container an horizontal scroll appear.
See this CodePen : https://codepen.io/zalog/pen/Rgrbod (first Navbar)

I'm not sure if it's really a bad behavior, maybe @mdo can help us in this use case ?

Thank you to @zalog to review/improve my first commit 👍

Close : #22699

@@ -77,9 +77,8 @@
}

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

Choose a reason for hiding this comment

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

What if we scoped this to a max-width media query instead? Would that mean we don't need to reset the styles later for .dropdown-menu and .dropdown-menu-right?

Copy link
Member Author

Choose a reason for hiding this comment

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

.dropdown-menu-right only exist here, because we handle that on our JS for the other Dropdown which aren't in a navbar.
But yes I think it's possible to add a media query just for the Dropdown position

Copy link
Contributor

@zalog zalog Jun 15, 2017

Choose a reason for hiding this comment

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

I'm on branch .v4-dropdown-navbar and made two pens:

  • one with the last commit: e836f5
  • the other with one commit before the last commit: 7d3368

The problem is that with our last commit:

.navbar-expand {
  @each $breakpoint in map-keys($grid-breakpoints) {
    $next: breakpoint-next($breakpoint, $grid-breakpoints);
    $infix: breakpoint-infix($next, $grid-breakpoints);

    &#{$infix} {
      @include media-breakpoint-down($breakpoint) {
        .dropdown-menu {
          position: static;
          float: none;
        }
...

the rules position: static; float: none; won't apply in case of:

  • navbars that always collapse .navbar
  • navbars that never collapse, .navbar-expand

This two use cases (navbar that always/never collapse) are already documented here.

I mentioned this, first time, here. I hope now It's much more clear :)

@Johann-S Johann-S force-pushed the v4-dropdown-navbar branch from 0dccebb to e836f59 Compare June 15, 2017 08:59
@Johann-S Johann-S added this to the v4.0.0-beta milestone Jun 15, 2017
@Johann-S
Copy link
Member Author

Johann-S commented Jun 15, 2017

Currently we have an issue on navbar (#22831) to fix before the merge of this PR
I misunderstood something but now it's clear sorry 😟

@Johann-S Johann-S force-pushed the v4-dropdown-navbar branch from e836f59 to 4e856a0 Compare June 15, 2017 10:53
return placement

if (this._inNavbar) {
popperConfig.modifiers.AfterApplyStyle = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason to make Popper apply styles and then remove them?

You could set the enabled property of the applyStyle modifier to !this._inNavbar

Copy link
Member Author

@Johann-S Johann-S Jun 15, 2017

Choose a reason for hiding this comment

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

Oh thank you @FezVrasta ! Nice review !

Edit :
Finally I tried what you suggested but it add inline style see floating-ui/floating-ui#309

Copy link
Contributor

Choose a reason for hiding this comment

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

Gonna fix this tomorrow morning!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants