search

bgrins / ExpandingTextareas

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

Refactor initialisation #55

Closed domchristie closed 9 years ago

domchristie commented 9 years ago

This reverts storing initialised Expanding instances in data (rather than in a registry) as it leads to more understandable code. (Takes some inspiration from twitter bootstrap plugins.)

It also removes the $.expanding.opts.update option. This is used to provide a default callback on update, but is not documented, and is a little confusing. Note: $('textarea').expanding({ update: function () {} }); is still supported.

.jshintrc is updated, and .js files have been updated to reflect these changes.

bgrins commented 9 years ago

When I load the home page with this applied, I'm not seeing the demo textarea expand for some reason

bgrins commented 9 years ago

(no errors in the console, happens on FF and Chrome)

domchristie commented 9 years ago

Sorry about that. One of the few things not covered by tests!

Fixed by https://github.com/domchristie/ExpandingTextareas/commit/ec35959994d85c8ac59ffb034aa768a9880ecbdd

bgrins commented 9 years ago

Sorry about that. One of the few things not covered by tests!

Ah, autoinitialization. We should probably have a test cover this

domchristie commented 9 years ago

Ah, autoinitialization. We should probably have a test cover this

I have attempted this, but it’s not straightforward given that the init code is called inside a jQuery document ready handler (any ideas?).

We could just test the presence of the correct $.expanding options, which would have failed in this case.

bgrins commented 9 years ago

We could just test the presence of the correct $.expanding options, which would have failed in this case.

Yeah, that would be a good place to start

I have attempted this, but it’s not straightforward given that the init code is called inside a jQuery document ready handler (any ideas?).

The test harness will be running before document ready has fired, so maybe we could do an async test that waits to resolve until after the DOM is ready. Registering a $(function() {}) inside the test should cause that one to fire after the one registered in expanding.js. You can call asyncTest to do this with qunit. Here is an example: https://github.com/bgrins/spectrum/compare/bgrins:e295dbe...bgrins:eb4cd0e#diff-38dcab2adf8a7e3431829d8daca050fcR658.

domchristie commented 9 years ago

I think the issue is that the #qunit-fixture element is reset after each test, meaning that an auto-initialized expanding textarea will lose its data after the first test. Checking for an active expanding textarea will fail unless the test is the first one that is run.

bgrins commented 9 years ago

Checking for an active expanding textarea will fail unless the test is the first one that is run.

If this is the case, I'd have no problem with just putting that test first with a comment about why it is there. Or even adding a new test html file just for this case.

domchristie commented 9 years ago

Or even adding a new test html file just for this case.

This is preferable since QUnit re-runs failing tests first, which can lead to odd results i.e. tests fail initially, then pass on refresh