bgrins / ExpandingTextareas

jQuery plugin for elegant expanding textareas
http://bgrins.github.com/ExpandingTextareas/
MIT License
261 stars 73 forks source link

Refactor to use constructor pattern #38

Closed domchristie closed 10 years ago

domchristie commented 10 years ago

This PR refactors the plugin to use the constructor pattern. It supercedes #37.

Apologies: there’s a lot going on. It seemed to make the most sense to bundle it as one PR.

There is one failing tests in IE8 (minor min-height discrepancy), and one failing in IE9 (clone border top/bottom width off by 1px). Neither seem to affect the perceived behaviour.

I totally understand if it’s too much, but thought I’d get it out there.

bgrins commented 10 years ago

I've tagged the current release as 0.0.1, and will look through this. I'm not opposed to refactoring the code base, but will have to check out the registry and constructor usage before merging. From first glance it looks pretty good though.

domchristie commented 10 years ago

OK, thanks: I appreciate there’s a lot going on!

bgrins commented 10 years ago

First of all - thanks! This looks great. I'm going to leave some notes inline

domchristie commented 10 years ago

That should be all the points covered.

Some further testing has revealed that triggering a custom event called update causes problems in IE when Prototype.js is included (see: http://bugs.jquery.com/ticket/14770)

Bit of an edge case I know, but thought it might be worth changing this event name. I know that resize was used before, but I’m not sure it really reflects what’s actually going on.

Or perhaps we don’t trigger an update event at all, and just call the callback directly?

bgrins commented 10 years ago

Some further testing has revealed that triggering a custom event called update causes problems in IE when Prototype.js is included (see: http://bugs.jquery.com/ticket/14770)

Weird... I always prefer triggering the event, than simply calling a callback, it makes it easier for people to use in different cases (like delegating events and stuff). We could call it resize or maybe reflow (which definitely shouldn't have any issues).

domchristie commented 10 years ago

The jQuery ticket has been updated, which includes the following:

This is because Prototype adds an .update() method to the element.

And references the jQuery docs:

Note: For both plain objects and DOM objects other than window, if a triggered event name matches the name of a property on the object, jQuery will attempt to invoke the property as a method if no event handler calls event.preventDefault(). If this behavior is not desired, use .triggerHandler() instead.

So perhaps .triggerHandler() is an option?

bgrins commented 10 years ago

So perhaps .triggerHandler() is an option?

Sure, that sounds good. Will pull down latest code now

bgrins commented 10 years ago

This looks great! The one thing I notice using it is that the 'reenable' button in the demo isn't working in the Destroying section of the home page

domchristie commented 10 years ago

b98aa1c fixes destroy example on homepage

bgrins commented 10 years ago

Just merged and tagged, thanks again! Were there other issues that can now be resolved with this change?

domchristie commented 10 years ago

Thanks!

27, #18 can also be closed, I believe