soundasleep / jquery-dropdown

Bootstrap-style dropdowns with some added features and no dependencies.
Other
767 stars 268 forks source link

Mixes library and actions in one script - less flexible #53

Closed judgej closed 10 years ago

judgej commented 10 years ago

The script that provides the functionality mixes both the jQuery extension and the binding of events to elements on the page. The two should be kept separate, as different sites will want to bind elements in different ways.

For example, the built-in binding uses custom attributes, when a single ID may suffice. It binds to the click events to toggle, when I may want to bind it to a focus/blue pair of events on a text field.

It is generally good practice to keep library functions that declare code separate from actions that run immediately; you wan to be able to include a plugin without it immediately leaping into action. The elements I want it to operate on may not even exist when the plugin is loaded (e.g. JS may add the drop-down block after the page has loaded), so I would need that control.

I would recommend taking the bindings out.

claviska commented 10 years ago

To clarify:

The elements I want it to operate on may not even exist when the plugin is loaded

The listener will work for dropdowns that are dynamically created—note the selector parameter of jQuery's on method.

This plugin is loosely based off of Bootstrap's dropdown plugin, which doesn't feature initialize/destroy methods—they're built to "just work". Although it's very common for jQuery plugins to require instantiation (see my MiniColors plugin), the Bootstrap paradigm doesn't follow that methodology. In fact, they've demonstrated rather successfully that the conservative use of namespaced listeners isn't intrusive or bad for performance in the majority of real world cases.

Some developers may argue that it's bad practice, lazy programming, or poorly developed software. Others might argue that it would be overkill to add init/destroy methods. Since this plugin was developed as a more flexible alternative to the 2.x Bootstrap dropdowns, I chose to maintain a similar API. Up until now, nobody has brought it up as an issue.

That said, if you have a reasonable use case showing why this approach fails or causes noticeable performance issues, it would definitely merit further discussion. Otherwise, the argument seems preferential :)

judgej commented 10 years ago

Fair enough. I'll probably fork the plugin and make some adjustments and see how it goes. I'm having conflicts with class names too, so will probably need to name-space the classname a little more uniquely too, and make the class names configurable options in the plugin.

My alternatives are bootstrap and jQuery-ui equivalents, but that is a whole new lot of additional bloat I would need to install, when this plugin gets straight to the single piece of functionality that I am looking for.

claviska commented 10 years ago

I'm open to better namespacing for classes, so when you get to that point feel free to submit a pull request :)