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): .dropdown-menu-right positioning #22707

Closed
wants to merge 1 commit into from

Conversation

zalog
Copy link
Contributor

@zalog zalog commented May 26, 2017

This is a fix for placing .dropdown-menu-right on the right position for .navbar and override Popper.js.

Problem: http://output.jsbin.com/sozuva/1
This PR: https://codepen.io/zalog/pen/BREOMv

cc #22636, #22640, #22699

@mdo
Copy link
Member

mdo commented May 26, 2017

I don't understand what this fixes exactly. The problem link here has no .dropdown-menu-right on the .dropdown-menu, and you shouldn't need any additional styles to override that since our navbar uses static positioning for dropdowns in narrower viewports.

@Johann-S
Copy link
Member

The jsbin use Tether and is outdated.
With our last dist file this is what we get : https://codepen.io/Johann-S/full/Pmgado/

@zalog
Copy link
Contributor Author

zalog commented May 27, 2017

Click on "Dropdown link", the bug is still there.
Please take a look on my pens:
before: https://codepen.io/zalog/pen/pPmzaE
after: https://codepen.io/zalog/pen/BREOMv

css is generated local: npm run css-compile & npm run css-prefix
js files loaded from cdnjs & rawgit. I used Popper.js 1.9.9

I rebased my branch.

@zalog zalog force-pushed the fix-dropdown-menu branch from ef798ac to c3b713c Compare May 27, 2017 13:14
@Johann-S
Copy link
Member

Yes it fixed the issue but on the last navbar for me static position should be applied only for narrower viewports.

@zalog
Copy link
Contributor Author

zalog commented May 28, 2017

That's a navbar that always collapse, a mobile style. I think the default behaviour for .dropdown-menu should open static on all the viewports sizes.
I belive it's much cleaner this way.

Or perhaps we find a way to get popper.js manage this, but I think we don't want all this mess on js.

I don't like override popper.js or use !important eighter...

@mdo, have some thoughts?

@mdo
Copy link
Member

mdo commented May 29, 2017

Ugh, yeah, we should work with the Popper team to get some better support for controlling the styles here. Having all these overrides kinda sucks. Ideally, instead of position: absolute; transform: translate3d(0px, 38px, 0px); top: 0px; left: 0px; will-change: transform; on every default dropdown, we apply as little as possible. There's no need for any of these given our CSS.

I'm unsure how best to rectify this in short order though.

@Johann-S
Copy link
Member

Maybe @FezVrasta have some ideas how we can handle that in a short term

@FezVrasta
Copy link
Contributor

Hey, Popper.js "team" here (😂).

If you want to make Popper.js apply only some specific styles you can inject a modifier before the applyStyle one and strip any unwanted properties from the styles object.

But in my opinion you should override the applyStyles modifier all together and create one that reads the class applied to the popper element and applies the needed styles accordingly

@Johann-S
Copy link
Member

Johann-S commented May 29, 2017

I will try to be a bit more precise :
- For me here they are no problem of positionning by Popper.js
- The problem is present on the first two navbars because of those lines : _navbar.scss#L79-L82 and _navbar.scss#L160-L162. Without those lines they are no problem of positionning
- What I currently don't understand is on those lines _navbar.scss#L79-L82 we add a static position without taking into account : it's for all the different viewports/breakpoint.
When I track the history of this file (_navbar.scss) we can see (ba2352c#diff-df1f74faf207a474f1d78c86393221c8) they were only one css rule where position were static and it was in a certain breakpoint and not for all the viewports/breakpoints 🤔 (maybe I misunderstood something here)

Maybe someone can help me to understand why now it's needed to have a static position for all the viewports/breakpoint and before it wasn't the case and they wasn't any issues about that.

Edit :
I spoke with @zalog on Slack and I understand all the problem 😄

@zalog zalog force-pushed the fix-dropdown-menu branch from c3b713c to 8094d9c Compare May 29, 2017 12:56
@zalog zalog force-pushed the fix-dropdown-menu branch from 8094d9c to 57b802c Compare June 1, 2017 00:57
@Johann-S
Copy link
Member

Johann-S commented Jun 4, 2017

I made a commit (see : ca60fe4) which reset Popper.js styles for Dropdowns in Navbar.
I don't know if it's the best solution but it fix our issue.

I post that here because I hope some feedbacks or ideas

@zalog
Copy link
Contributor Author

zalog commented Jun 7, 2017

We have now 2 ways of fixing this:

In my opinion we shoud step the Johann way :)

@Johann-S
Copy link
Member

Johann-S commented Jun 8, 2017

Superseded by #22782

@Johann-S Johann-S closed this Jun 8, 2017
@zalog zalog deleted the fix-dropdown-menu branch June 8, 2017 20:33
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.

4 participants