Clear input button for text inputs #5281

Merged
merged 5 commits into from Nov 20, 2012

2 participants

@collinforrester

This is a change to four files (2 test files and then the js/css for the text input widget) to add feature request #1834 (#1834) to automatically add a text clear button to text inputs with data-clear-button=true.

May need to decide if its right to allow the clear button on textarea elements or if only on input type text (right now it does both).

Collin F and others added some commits Nov 13, 2012
Collin F text input: modified css to add clear button to input type text. Feat…
…ure request #1834 - clear input button for text inputs
8c8c3f9
Collin F text input: modified text input widget to include clear button on inp…
…ut type text. Feature request #1834 - clear input button for text inputs
eeb69a0
Collin F text input: modified test to include new text input type. Feature req…
…uest #1834 - clear input button for text inputs
b624b6f
@jaspermdegroot
jQuery Foundation member

@collinforrester

Thanks for the PR! I reviewed your code and created a test page. Here is my feedback:

  • clearBtn: false should be added to options so you can configure the default or set the option programmatically, not only with the data- attribute.
  • With your code the clear button is added if you don't set the data- attribute at all. After making clearBtn an option you can use if ( input.is( "[type='text']" ) && !!o.clearBtn ) { }. The widget factory will check if data-clear-btn has been set.
  • I think we should introduce option clearButtonText and deprecate clearSearchButtonText.
  • Except for the variable focusedEl the code for adding the clear button is exactly the same as for "search" so we shouldn't have to duplicate everything.
  • Text inputs and textarea should get the ui-corner-all class instead of ui-btn-corner-all.
  • The disabled styling doesn't work anymore.
  • The width is incorrect when wrapped in a div with data-role="fieldcontain".
  • There are some conflicts with native browser controls for input types number, date, etc. (I tested on Chrome). I am not sure yet what we should do with those.
  • The slider widget has a number input that is enhanced by the textinput widget. In this case clearBtn should always be `false.

I know it is quite a lot. Do you want to look into this?

@collinforrester

No problem, I'll keep looking into this. I'll go through the list and commit some changes for each one.

Thanks for the quick review, too.

@collinforrester

@uGoMobi

So I committed some stuff that addresses everything but your last two bullets in your original review (I just noticed I didn't read your comment about the slider widget). But I made those changes to affect only search, text, and textarea until you guys decide what you want to do with the other input types in #1834.

@jaspermdegroot
jQuery Foundation member

hi @collinforrester

Thanks a lot! I pulled your changes in the branch that I created, so the test page is updated... looks good!

Had to make a small change because of an undeclared variable. Also, at second thought clearBtnText is a more consistent name for the option, so I changed that. See my commits here https://github.com/jquery/jquery-mobile/commits/text-input-clear-btn

I will take a closer look at the JS and CSS soon and will let you know what we want to do with the number and date input.

Thanks again!

@jaspermdegroot jaspermdegroot merged commit e1bd316 into jquery:master Nov 20, 2012
@jaspermdegroot
jQuery Foundation member

@collinforrester

I just merged your commits in master.

I made a few changes. We think the clear button shouldn't be optional for "search". I enabled the option for all input types. We still have to look into date, number, color and other HTML5 input types, but those can be added to the blacklist. We also decided not to add the feature for textareas because this is not really common, and you are not happy when you accidently touch the clear button after typing long message on your phone ;-)

Thanks a lot for all your work on this new feature! Looking forward to your next PR! :-)

@zachleat zachleat referenced this pull request in NebraskaJS/Wiki Nov 29, 2012
Closed

January 3rd Presentation Topic #1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment