Textinput: Only _destroy() clearButton when present #7568

gabrielschulhof · 2014-07-15T14:03:14Z

arschmitz · 2014-07-30T19:16:55Z

tests/unit/textinput/textinput_core.js

@@ -149,4 +149,35 @@
 			"turning off clearBtn removes wrapper class 'ui-input-has-clear'" );
 	});

+( function() {


Why is this in its own iife?

Well, mostly because I declare variables that are relevant only to the test that follows. I'd hate to have to declare those variables at the top of the file (as per our style guide), because having them placed so far away from where they're actually used is confusing. However, it's not just for aesthetics. The scoping reflects the fact that the module setup/teardown is not enough to actually gather the information that I need.

arschmitz · 2014-08-21T17:18:49Z

tests/unit/textinput/textinput_core.js

+		deepEqual( $.testHelper.domEqual( originalDOM, $( "#destroy-test-container" ) ), true,
+			"Original DOM is restored after textinput destruction" );
+
+		deepEqual( destroyClearCalled, false, "_destroyClear() was not called" );


This test does not really make sense it cant possibly run AND evaluate to true because there will be a failure because of the error being thrown.

OK, I totally misread your comment here. I thought you did not want me to let the exception bubble up to qunit and have the test fail that way.

There is still a good reason to ensure that _destroyClear() does not get called. If a text input does not have a clear button, _destroyClear() should not get called, whether it throws an exception or not. So, I think the second assertion is justified.

If you accept this line of reasoning, I'll restore this test and delete the one below.

Closes jquery-archivegh-7568 Fixes jquery-archivegh-7567

Closes gh-7568 Fixes gh-7567

arschmitz · 2014-08-26T11:13:30Z

tests/unit/textinput/textinput_core.js

+	test( "textinput destruction does not throw error", function() {
+		var errorThrown;
+
+		try {


QUnit already does a try catch there is no need to test for errors in tests. If an error is triggered the tests will fail and it will be caught by qunit

gabrielschulhof · 2014-08-27T11:28:31Z

@arschmitz I've answered your comment on the outdated diff because that version of the test may be the way to go after all.

Argh! To see the outdated diff click on the link above, then go to the location and hit Enter, otherwise you won't see the actual diff and the discussion.

arschmitz · 2014-08-27T16:22:59Z

@gabrielschulhof you don't need to test that the method is not called or catch any errors just test destroy on a textarea and an input with out a clearbutton if you don't get an error its not called. If you get an error the test fails.

gabrielschulhof · 2014-08-27T19:23:53Z

OK. Gotcha. In that case, the destroy test alone should suffice.

Closes gh-7568 Fixes gh-7567

arschmitz · 2014-09-04T12:36:58Z

js/widgets/forms/clearButton.js

@@ -127,7 +127,9 @@ define( [

 		_destroy: function() {
 			this._super();
-			this._destroyClear();
+			if ( this.options.clearBtn && !this.options.enhanced ) {


We still do normal destroy when enhanced is true

Riiight! True!

Wait ... I don't get it. I call super() ... let's talk on IRC ...

arschmitz · 2014-09-04T17:35:26Z

👍

(cherry picked from commit c6ba792) Closes gh-7568 Fixes gh-7567

Closes jquery-archivegh-7568 Fixes jquery-archivegh-7567

gabrielschulhof added Action: PR needs reviewing labels Jul 15, 2014

gabrielschulhof added this to the 1.4.4 milestone Jul 15, 2014

gabrielschulhof assigned arschmitz Jul 15, 2014

arschmitz reviewed Jul 30, 2014
View reviewed changes

arschmitz added Action: PR needs discussion and removed Action: PR needs reviewing labels Jul 31, 2014

arschmitz assigned gabrielschulhof and unassigned arschmitz Jul 31, 2014

gabrielschulhof added Action: PR needs modification and removed Action: PR needs modification labels Aug 1, 2014

gabrielschulhof assigned arschmitz and unassigned gabrielschulhof Aug 1, 2014

arschmitz reviewed Aug 21, 2014
View reviewed changes

arschmitz assigned gabrielschulhof and unassigned arschmitz Aug 21, 2014

arschmitz added Action: PR needs modification and removed Action: PR needs reviewing labels Aug 21, 2014

gabrielschulhof pushed a commit to gabrielschulhof/jquery-mobile that referenced this pull request Aug 23, 2014

Textinput: Only _destroy() clearButton when present

3560274

Closes jquery-archivegh-7568 Fixes jquery-archivegh-7567

gabrielschulhof force-pushed the 7567-fix-textinput-destroy branch from 8eab5fd to a11be3f Compare August 26, 2014 08:39

gabrielschulhof pushed a commit that referenced this pull request Aug 26, 2014

Textinput: Only _destroy() clearButton when present

a11be3f

Closes gh-7568 Fixes gh-7567

gabrielschulhof added Action: PR needs reviewing and removed Action: PR needs modification labels Aug 26, 2014

gabrielschulhof assigned arschmitz and unassigned gabrielschulhof Aug 26, 2014

arschmitz reviewed Aug 26, 2014
View reviewed changes

arschmitz removed their assignment Aug 26, 2014

arschmitz assigned gabrielschulhof Aug 26, 2014

arschmitz added Action: PR needs modification and removed Action: PR needs reviewing labels Aug 26, 2014

gabrielschulhof added Action: PR needs reviewing and removed Action: PR needs modification labels Aug 27, 2014

gabrielschulhof assigned arschmitz and unassigned gabrielschulhof Aug 27, 2014

Gabriel Schulhof added 2 commits August 27, 2014 22:30

Textinput: Only _destroy() clear button when present

cb1f649

Closes gh-7568 Fixes gh-7567

Textinput: Test destruction

f31a93a

gabrielschulhof force-pushed the 7567-fix-textinput-destroy branch from a11be3f to f31a93a Compare August 27, 2014 19:30

arschmitz reviewed Sep 4, 2014
View reviewed changes

arschmitz added Action: PR needs modification and removed Action: PR needs reviewing labels Sep 4, 2014

arschmitz assigned gabrielschulhof and unassigned arschmitz Sep 4, 2014

Textinput: Remove clear button even if pre-rendered

1a7bc89

gabrielschulhof added Action: PR needs reviewing and removed Action: PR needs modification labels Sep 4, 2014

gabrielschulhof assigned arschmitz and unassigned gabrielschulhof Sep 4, 2014

gabrielschulhof closed this in c6ba792 Sep 4, 2014

gabrielschulhof pushed a commit that referenced this pull request Sep 4, 2014

Textinput: Only _destroy() clear button when present

7f3f585

(cherry picked from commit c6ba792) Closes gh-7568 Fixes gh-7567

gabrielschulhof deleted the 7567-fix-textinput-destroy branch September 4, 2014 20:28

agcolom pushed a commit to agcolom/jquery-mobile that referenced this pull request Nov 26, 2014

Textinput: Only _destroy() clear button when present

0da5e0c

Closes jquery-archivegh-7568 Fixes jquery-archivegh-7567

jaspermdegroot removed the Action: PR needs reviewing label Aug 20, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Textinput: Only _destroy() clearButton when present #7568

Textinput: Only _destroy() clearButton when present #7568

Uh oh!

gabrielschulhof commented Jul 15, 2014

Uh oh!

arschmitz Jul 30, 2014

Uh oh!

gabrielschulhof Aug 1, 2014

Uh oh!

arschmitz Aug 21, 2014

Uh oh!

gabrielschulhof Aug 27, 2014

Uh oh!

arschmitz Aug 26, 2014

Uh oh!

gabrielschulhof commented Aug 27, 2014

Uh oh!

arschmitz commented Aug 27, 2014

Uh oh!

gabrielschulhof commented Aug 27, 2014

Uh oh!

arschmitz Sep 4, 2014

Uh oh!

gabrielschulhof Sep 4, 2014

Uh oh!

gabrielschulhof Sep 4, 2014

Uh oh!

arschmitz commented Sep 4, 2014

Uh oh!

Uh oh!

Textinput: Only _destroy() clearButton when present #7568

Textinput: Only _destroy() clearButton when present #7568

Uh oh!

Conversation

gabrielschulhof commented Jul 15, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gabrielschulhof commented Aug 27, 2014

Uh oh!

arschmitz commented Aug 27, 2014

Uh oh!

gabrielschulhof commented Aug 27, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arschmitz commented Sep 4, 2014

Uh oh!

Uh oh!