Skip to content
This repository has been archived by the owner on Oct 8, 2021. It is now read-only.

Clear input button for text inputs #5281

Merged

Conversation

collinforrester
Copy link
Contributor

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 3 commits November 13, 2012 12:51
@ghost ghost assigned jaspermdegroot Nov 13, 2012
@jaspermdegroot
Copy link
Contributor

@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
Copy link
Contributor Author

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
Copy link
Contributor Author

@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
Copy link
Contributor

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-archive:master Nov 20, 2012
@jaspermdegroot
Copy link
Contributor

@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! :-)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants