Bug Tracker

Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#3498 closed enhancement (fixed)

Add mouseenter and mouseleave shortcuts

Reported by: phiggins Owned by: flesler
Priority: minor Milestone: 1.3
Component: event Version: 1.2.6
Keywords: Cc: flesler
Blocked by: Blocking:

Description

.hover requires two parameters. It might be work considering delegating one function between the two events by only registering a single function.

also, .mouseenter and .mouseleave are omitted from the list of convenience methods attached to .bind().

attached patch resolves both.

Attachments (1)

mouseenter.diff (827 bytes) - added by phiggins 11 years ago.
shorten hover, add optional second param, add mouseenter/leave methods

Download all attachments as: .zip

Change History (18)

Changed 11 years ago by phiggins

Attachment: mouseenter.diff added

shorten hover, add optional second param, add mouseenter/leave methods

comment:1 Changed 11 years ago by flesler

need: ReviewCommit
Status: newassigned

comment:2 Changed 11 years ago by flesler

Component: unfilledevent

comment:3 Changed 11 years ago by flesler

Resolution: fixed
Status: assignedclosed

I agree about $.fn.mouseenter/mouseleave, but definitely not about 1 function for both events.

Applied first part at [5895].

That's not generic enough and can be unexpected for some users.

You can simply do that by: $(..).bind('mouseenter mouseleave',someFn);

comment:4 Changed 11 years ago by phiggins

great - the delegation seems like it could be something taught as a best practice, though admittedly the implementation is silly ... it seems something you can teach inadvertently through .hover, but if you really want to explain the space-separated-bind-events, who am I to argue. Thanks for applying the patch. My stuff is AFL/BSD, if it matters (and it should)

regards- phiggins

comment:5 Changed 11 years ago by phiggins

to be clear, I wasn't changing the API in place at all - just offering the advanced users an option to reuse a single function -- it seems convenient to "be able to", rather than the terse syntax of registering the function twice manually -- and it only adds 5 bytes above the current patch to allow the option. unless you were reserving a null second param to hover for something more significant ... I like the shorthand myself.

comment:6 Changed 11 years ago by phiggins

also, not to nitpick - but even without the optional second parameter, it would be smaller to use my .mouseenter/.leave patch with no options than .bind('mouseenter')/.bind("mouseleave") even when considering how gzip works.

comment:7 Changed 11 years ago by kswedberg

heh. I've been meaning to add a ticket for .mouseenter() and .mouseleave() for months. glad someone else took the bull by the horns, so to speak. thanks, phiggins.

comment:8 in reply to:  4 Changed 11 years ago by john

Replying to phiggins:

My stuff is AFL/BSD, if it matters (and it should)

Wait... you provided a patch, on the jQuery project tracker, with no specified license and you are now retroactively adding an incompatible license to the patch after it had already been applied?

Please tell that there's some miscommunication going on here.

comment:9 Changed 11 years ago by john

Cc: john added

comment:10 Changed 11 years ago by phiggins

no I was being cheeky -- I suppose I supplied the patch as mit/gpl, but typically do my stuff in bsd/afl. there is no retroactive or anything shady going on, just open source.

either way, the patch is enough of a no-brainer not to carry any ip imho

regards Peter Higgins

comment:11 Changed 11 years ago by flesler

First time I hear about licensing for a patch for jQuery :)

Anyway...

also, not to nitpick - but even without the optional second parameter, it >would be smaller to use my .mouseenter/.leave patch with no options than >.bind('mouseenter')/.bind("mouseleave") even when considering how gzip works.

Right, I forgot to use the shorthands... do I need to worry about licensing ? :D

it seems convenient to "be able to", rather than the terse syntax of >registering the function twice manually -- and it only adds 5 bytes above >the current patch to allow the option.

I agree that it's a small change. The problem is that many users think they can just send 1 fn to hover() assuming the 2nd is set to an empty function. On this situation, they could get unexpected side effects.

comment:12 Changed 11 years ago by phiggins

re: shorthands / licensing: no. a) is a no-brainer patch and b) in the full spirit of oss, sorry I even mentioned the word :P

re: second param: It really is a matter of documentation at this point, unless I'm unclear as to what .bind('mouseleave', undefined) would result in. seems like it throws an error, but by your description it would fail silently? Maybe i'm just confused. Are you saying that $(.).hover(function(){..}) only registers a .mouseenter()? or is it documented otherwise and you are trying to keep the errors thrown when passing only one?

Allowing both to be on the first position would confuse someone I agree, but if clearly documented it becomes moot, and a handy feature. Unless the exception throwing is a way of telling people "you need two functions here". The documents only state you need both, and in the case of anonymous functions putting both on one callback seems something worthwhile, though not something worth arguing over ;)

comment:13 in reply to:  12 Changed 11 years ago by flesler

Replying to phiggins:

re: second param: It really is a matter of documentation at this point, unless I'm unclear as to what .bind('mouseleave', undefined) would result in. seems like it throws an error, but by your description it would fail silently? Maybe i'm just confused. Are you saying that $(.).hover(function(){..}) only registers a .mouseenter()? or is it documented otherwise and you are trying to keep the errors thrown when passing only one?

No, I know it fails, but that makes the user realize they made a mistake. If we'd silently bind the function twice, they wouldn't know. We had indeed documented that hover requires 2 functions. Still, so many users ignore that. I think silently binding the fn twice would add more side effects than advantages. Considering that you can use the shortcut I proposed that is a few bytes longer, and also that this doesn't seem to be useful in too many situations.

comment:14 Changed 11 years ago by phiggins

ahh claro - I agree then. but it still should be:

hover: function(over, out){ this.onmouseenter(over).onmouseleave(out) },

in the interest of size

comment:15 Changed 11 years ago by flesler

Added the shortcuts. Thanks

comment:17 Changed 11 years ago by flesler

Cc: flesler added; john removed
Summary: enhancement: Allow for optional second function on hover + add .mouseenter/leave funcAdd mouseenter and mouseleave shortcuts

comment:18 Changed 11 years ago by brandon

This was a lively ticket. FYI... the one function to hover enhancement was applied in r6342. Ticket #4234.

Note: See TracTickets for help on using tickets.