-
Notifications
You must be signed in to change notification settings - Fork 503
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
Prevent hover from acting on navbar dropdowns in collapsed mode #75
base: master
Are you sure you want to change the base?
Conversation
Added a setting for navbar breakpoint in defaults Added check in openDropdown to see if dropdown is a descendant of .navbar-collapse and window width is less than the navbar breakpoint
changed from tabs to 4 spaces
Do you think we should then get rid of the if('ontouchstart' in document) return this; line? |
I don't think so. That line is to disable the plugin on touch enabled devices. My updates are to disable the plugin when the menu is in collapsed mode (i.e. acting like an accordion). |
What about #68? |
Well, like I say, it's a different issue. item 1 In collapsed mode, when you move the mouse off the bottom of item 1-sub 3 the menu collapses down, and you will skip over item 2 entirely (maybe item 3 as well, not testing right now). |
Here's an example of the effect I'm discussing http://www.bootply.com/3dPu0COV1A |
So is the use case people with really really small laptops? |
I'll admit it is a less common use case (I'm not sure if I noticed it before reading issue #75). |
Just running into the same issue. I also admit to having more than one window open :-) To eliminate the extra needed config for the breakpoint i would change the Addendum: Also the delayed close after mouseover (line 57) needs this check. Otherwise the menu items in the collapsed menu close automatically. |
Instead of having a variable for nav breakpoint we now check if .navbar-toggle is visible. We also have added the check to the close on mouseout, so the menu doesn't close when we hover outside the submenu. Made a function isCollapsed so we can have our logic in one place.
Was: if ( isCollapsed ){ return; } Now: if ( isCollapsed() ){ return; }
I've incorporated the changes suggested by Sloothword. |
This is looking better. I will try and go over it and see how it behaves I am nervous to basically change anything at this point without getting some tests to make sure we don't regress. So many little edge cases! See #69. |
|
So does this not break buttons (stuff not in navbars) when on mobile? |
@CWSpear can't imagine why this should break any stuff outside the navbar!? |
Sorry, I was thinking about it backwards. But looking at this PR, if you don't have a navbar, etc, $(".navbar-toggle").filter(":visible").length will always be 0, and mess up that |
@CWSpear you are right - didn't think about. But this should be fixable easily. I'm not so into JavaScript but how about a first check if the .navbar-toggle exist and then do the length check? |
@CWSpear Is your concern that some people will have a navbar without a toggle icon? I'm not sure of a solution for that, but remember that the isCollapsed function works to -disable- the plugin. If there is no .navbar-toggle, then it will return false, and thus the items you hover over -will- dropdown. @gadgetto $(".navbar-toggle").filter(":visible").length basically means "if there is at least one .navbar-toggle, which is visible". |
@neil-h ok, I see what you're saying about it really only affecting the nav bar, but it looks like it is still a little dangerous. By the way, this issue goes away if we use PointerEvents (see commit 6d163831). That approach requires PointerEvents tho (duh), which are currently only supported on IE, but coming to Chrome and Firefox, and requires jQuery's PointerEvent Polyfill in the interim. However it's the only solution I've found that really solves issues like this one and #68 in a real, solid manner. This sort of stuff is also very hard to test in an automated fashion, and one of the goals for v3.0 of this plugin is tests. We haven't been able to really get any help on that front, and so the going is very slow. |
this commit fixes #73
This will prevent hover from activating a dropdown in a navbar in collapsed mode.
If someone has set their navbar collapse breakpoint to something other than the default, then they will need to enter that into the settings.