Bug Tracker

Opened 11 years ago

Closed 11 years ago

Last modified 10 years ago

#11786 closed enhancement (fixed)

Deprecate .toggle( handler, handler, ... ) signature

Reported by: gibson042 Owned by:
Priority: low Milestone: 1.8
Component: event Version: 1.7.2
Keywords: Cc:
Blocked by: Blocking:

Description

Did you know we have two .toggles? The one we like performs .show or .hide as appropriate, and is documented at http://api.jquery.com/toggle/. The other sets up round-robin .click handlers, and is documented at http://api.jquery.com/toggle-event/:

The .toggle() method is provided for convenience. It is relatively straightforward to implement the same behavior by hand, and this can be necessary if the assumptions built into .toggle() prove limiting. For example, .toggle() is not guaranteed to work correctly if applied twice to the same element. Since .toggle() internally uses a click handler to do its work, we must unbind click to remove a behavior attached with .toggle(), so other click handlers can be caught in the crossfire. The implementation also calls .preventDefault() on the event, so links will not be followed and buttons will not be clicked if .toggle() has been called on the element.

Let's put it out of our misery.

Change History (8)

comment:1 Changed 11 years ago by dmethvin

Here are some thoughts:

Against removing:

  • It's been around a while, and will break code if removed.

For removing:

  • The name .toggle() is ambiguous, and of the two uses this is by far the more obscure.
  • All the strange quirks mentioned in the docs.
  • It doesn't depend on internals and could be a plugin.
  • Saves 132 bytes gzipped. (That surprised me!)

comment:2 Changed 11 years ago by Rick Waldron

Dammit Dave! You blitzed me!

comment:3 Changed 11 years ago by dmethvin

Component: unfiledevent
Milestone: None1.8
Priority: undecidedlow
Resolution: fixed
Status: newclosed

As @gibson042 mentions, the documentation is practically a suicide note. Marked for deprecation in the docs, we'll create a new ticket for removal when that happens.

comment:4 Changed 11 years ago by mplungjan

Can someone please guide me to the official deprecation notices?

I understand the need to disambiguate but where are the warnings? Neither in the documentation [here](http://api.jquery.com/toggle-event/) nor [here](http://jquery.com/upgrade-guide/1.9/)

http://stackoverflow.com/questions/14301935/where-has-fn-toggle-handlereventobject-handlereventobject-gone

comment:5 Changed 11 years ago by dmethvin

Unfortunately the "Deprecated" category at api.jquery.com got lost during the transition to a new server in the past couple of weeks. I think kswedberg is working on restoring that.

The missing mention in the upgrade guide is my oversight, I'll put one in now. The jQuery Migrate plugin does have a warning for it, you'll see it described here.

https://github.com/jquery/jquery-migrate/blob/master/warnings.md

comment:6 Changed 11 years ago by Mplungjan

Would it not be great to re-use the name in the Migrate and add it back in as fn.toggler ?

comment:7 Changed 11 years ago by dmethvin

It's not really an appropriate core function. If someone has code that uses it already the Migrate plugin will let them continue to do so without changing any code. The function with another name would still have the drawbacks it does today, including not working with delegation and having no obvious way to remove it. If someone wants the code it's there in the Migrate plugin and can be extracted.

Last edited 11 years ago by dmethvin (previous) (diff)

comment:8 Changed 10 years ago by cougar.b@…

Just a comment from a newbie. I was tearing my hair out for hours and hours, comparing code that worked with that which didn't and finding no difference, before cluing in to the idea that I had upgraded my requirejs, and that this might be the problem. After eventually finding that toggle had been deprecated and removed, I searched the internet for a fix, and nothing worked. So I read your change log, clicked on this ticket number, and there, in comment #5, is a reference to the Migrate plugin. But the Github link was unhelpful.

I searched jQuery and found the Migrate plugin link, which provided me with a fix: adding one line that referenced the Migrate plugin. But I still don't know what a direct path to this knowledge would have been. Maybe I'm just too much of a newbie, but my opinion is that if you deprecate and remove things that you know will break code, there should be more obvious links to Migrate. All in all, this cost me 15 hours and six gray hairs--at least on the top of my head...

I now know that when I eventually replace the offending code with a module, I'm going to have to completely rewrite the functionality, and I can do that in a sandbox, rather than breaking things. It would have been nice to have more help with preventing this problem in the first place.

Note: See TracTickets for help on using tickets.