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

Dropdowns update - keyboard, touch and submenus #6048

Closed
wants to merge 1 commit into from
Closed

Dropdowns update - keyboard, touch and submenus #6048

wants to merge 1 commit into from

Conversation

blakeembrey
Copy link
Contributor

Fixes:

  • Keyboard navigation using submenus (acts similar to right-click in Mac, going up first starts to top of list, bottom the bottom - moves from there, etc. Only difference is that the submenus display on item focus)
  • Takes into account the .dropup, .dropdown and .pull-left modifier classes. E.g. If it's pull-left the left and right arrows switch around, if it's a dropup submenu - we start by focusing the last item vs the first which is at the top.
  • Touch navigation now works prevents the default click events from happening with the delay
  • [esc] key will bring you back in focus with the initial button no matter where in the dropdown you are
  • Adds a item visibility selector to only navigate through visible items

@blakeembrey
Copy link
Contributor Author

This is just a copy of #5067 but rebased against 2.2.2-wip

…into account .pull-left, .dropup, .dropdown modifier classes). Takes into account whether we entered the menu using up or down. Behaves much like a standard dropdown menu on Mac, minus the submenu showing on focus. As a result of this - .pull-left will cause navigation to the submenu to be the left arrow v right normally. Use opposite direction to go up a level. Use [esc] will bring you back to the initial button focused. Reenables and adds touch support to menus and submenu items.

Conflicts:
	js/bootstrap-dropdown.js
@blakeembrey
Copy link
Contributor Author

@mdo There are a couple of things I would like to verify with the PR.

The first, the click event never actually occurs - it will be intercepted by the touchstart which triggers a click. Is this a way Bootstrap would want to approach something like dropdown menus? It wouldn't match other plugin behaviours so I am hesitant about this. Plus, it won't give the usual 300ms delay for clicks (which may or may not be a good thing).

The second is, the touch and focus functionality is almost identical and I would like to combine it into a single function. What is the stance from Bootstrap's code standard point of view - too much magic in a single function if I did this?

Thanks

@mdo
Copy link
Member

mdo commented Dec 2, 2012

Unsure about those aspects, we'll have to talk to @fat.

While we're looking at keyboard navigation though, any thoughts on #5990?


if (!/(38|40|27)/.test(e.keyCode)) return
if (!~$.inArray(e.which, [27, 37, 38, 39, 40])) return
Copy link
Member

Choose a reason for hiding this comment

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

why change this from regex

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No reason in particular - I think it was because I was looking through the typahead source and matched the functionality there (or perhaps another file). Either should be fine.

E.g. https://github.com/twitter/bootstrap/blob/master/js/bootstrap-typeahead.js#L220

Copy link
Member

Choose a reason for hiding this comment

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

alright, change this back to the regex style, it looks nicer

@blakeembrey
Copy link
Contributor Author

@mdo Not sure if this fix is going into the 2.2.2 patch, but there are some things I want to improve upon. Mainly combining the focus and touch events into one function. I was also looking at incorporating some of the functionality mentioned from #4527

If it's not going into 2.2.2, feel free to close this PR and I'll update the plugin for 2.2.3.

Thanks

@blakeembrey
Copy link
Contributor Author

Also thinking of changing the click functionality back to being a click instead of touchstart for consistency with the rest of the plugins.

@@ -26,11 +26,11 @@
/* DROPDOWN CLASS DEFINITION
* ========================= */

var toggle = '[data-toggle=dropdown]'
var toggle = '[data-toggle="dropdown"]'
Copy link
Member

Choose a reason for hiding this comment

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

remove the double quotes

@fat
Copy link
Member

fat commented Dec 7, 2012

this is a big patch (nearly doubles the size of the plugin)

The more i look at it, the more I think it would be better if I took this on myself. I appreciate you giving it a go though. thanks!

@fat fat closed this Dec 7, 2012
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