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

Forward originalEvent to hide.bs.dropdown handlers #11373

Closed
wants to merge 4 commits into from

Conversation

amigrave
Copy link

@amigrave amigrave commented Nov 5, 2013

One might be interested to check the original event that originally triggered the dropdown's custom event.

In my case I need the target of the original click event in order to decide if I preventDefault or not.

@@ -90,12 +90,14 @@
$items.eq(index).focus()
}

function clearMenus() {
function clearMenus(ev) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Presumably the call to clearMenus() within toggle() should pass along its event object?

Copy link
Author

Choose a reason for hiding this comment

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

Correct me if I'm wrong but toggle is supposed to be called programmatically so I don't see what event could be bound to it !?

Copy link
Collaborator

Choose a reason for hiding this comment

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

toggle is bound to the dropdown's click event, and is invoked when you click a dropdown to show/hide its menu.

Copy link
Author

Choose a reason for hiding this comment

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

My bad ! I applied the same principle to all custom events triggered by dropdown. I also used sourceEvent instead of originalEvent in order to avoid to hoist the jQuery semantic on originalEvent. If you prefer another name than sourceEvent just tell me.

@cvrebert
Copy link
Collaborator

cvrebert commented Nov 5, 2013

Also, documentation should be added for this.

Renamed originalEvent to sourceEvent
After having a look at the jQuery mouseenter event I could
see that originalEvent is not necessary the same as the current event.
So it make sense to use the same convention as jQuery.
@amigrave
Copy link
Author

amigrave commented Nov 6, 2013

Ok, the events triggered by dropdown are now consistent and the documentation has been updated as a more advanced code example.

As English is not my native language you'd better check the wording.

@amigrave
Copy link
Author

Bumping this pull request.

@cvrebert
Copy link
Collaborator

For consistency with the modal events, we should probably just pass the target of the original event as relatedTarget rather than passing the entire original event.

@cvrebert
Copy link
Collaborator

Anyway, due to versioning constraints, we can't merge this until Bootstrap v3.1, so expect to wait a while.

@fat fat closed this in 76f0d0f Dec 30, 2013
@mdo mdo mentioned this pull request Dec 31, 2013
@amigrave
Copy link
Author

Shall I do something about this ? I don't know what this "closed with unmerged commits" message means.

@cvrebert
Copy link
Collaborator

@amigrave Your pull request wasn't merged, but @fat instead wrote his own code to implement the feature (see above commit).

kevinawoo added a commit to Loopfirst/bootstrap that referenced this pull request Mar 28, 2014
* upstream/master: (119 commits)
  Drop trailling comma
  New Year
  fix assets links in all examples
  Update Gruntfile.js to copy 'dist/' files to 'docs/dist/' with 'grunt dist' task; Fixes twbs#12030: navbar toggle focus state
  Fix broken JS
  derp
  Fixes twbs#12046: move .csscomb.json and .csslintrc to less/ folder
  typo
  @nschonni's feedback <3
  @cvrebert's feedback
  Update dependencies (again)
  Fixed typo
  Sauce now supports latest Firefox on OS X Mavericks
  default to latest Firefox version on OS X
  fix capitalization of iOS
  mv sauce_browsers.yml out of the project root
  add note about ios dropdown compat
  fixes twbs#11379 - Fix carousel this.sliding not getting reset if $next.hasClass('active')
  fixes twbs#11373 - adds related target to dropdown events
  fixes twbs#11288 - Vertical scroll position of modal saves between openings
  ...

Conflicts:
	Gruntfile.js
	dist/css/bootstrap-theme.css
	dist/css/bootstrap-theme.css.map
	dist/css/bootstrap-theme.min.css
	dist/css/bootstrap.css
	dist/css/bootstrap.css.map
	dist/css/bootstrap.min.css
	dist/js/bootstrap.js
	dist/js/bootstrap.min.js
	docs/assets/css/docs.css
	docs/assets/css/pygments-manni.css
	docs/assets/ico/apple-touch-icon-144-precomposed.png
	docs/assets/js/application.js
	docs/assets/js/customize.js
	docs/assets/js/customizer.js
	docs/assets/js/filesaver.js
	docs/assets/js/holder.js
	docs/assets/js/ie8-responsive-file-warning.js
	docs/assets/js/jszip.js
	docs/assets/js/less.js
	docs/assets/js/raw-files.js
	docs/assets/js/uglify.js
	test-infra/s3_cache.py
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.

None yet

2 participants