Make .placeholder classname configurable #39
Conversation
|
The easiest solution would be to just use an argument for this, e.g. I was thinking of using Would you mind tweaking your pull request so it works that way? |
| @@ -14,7 +14,10 @@ | |||
|
|
|||
| } else { | |||
|
|
|||
| $.fn.placeholder = function() { | |||
| var placeholderClassName = 'placeholder'; | |||
There was a problem hiding this comment.
I’m not a fan of variable declarations inside an if { … } else { … } — better to keep them at the top of the scope IMHO. #ocd
|
Ping. I guess there's no notification for commit additions to a pull request? :/ |
| @@ -2,7 +2,8 @@ | |||
| ;(function(window, document, $) { | |||
|
|
|||
| var isInputSupported = 'placeholder' in document.createElement('input'), | |||
| isTextareaSupported = 'placeholder' in document.createElement('textarea'); | |||
| isTextareaSupported = 'placeholder' in document.createElement('textarea'), | |||
| placeholderClassName = 'placeholder'; | |||
There was a problem hiding this comment.
This line isn’t needed, is it?
There was a problem hiding this comment.
It's used in clearPlaceholder and setPlaceholder functions, plus the domready and unload event handlers.
There was a problem hiding this comment.
What I was thinking is that this should be added to the code after $.fn.placeholder is declared:
$.fn.placeholder.className = 'placeholder';
There was a problem hiding this comment.
Ah I see, so I should only get the className via $.fn.placeholder.className instead of a variable (outside of the function scopes)?
There was a problem hiding this comment.
Yeah, and cache/re-use the $.fn.placeholder.className wherever possible (per scope).
We should probably move the$(…) and the $(window).bind(…) code inside of the $.fn.placeholder = fn block, while still making sure these only get called/bound once (even when $().placeholder() is used multiple times). We could use .one() instead of .bind(), but what about the $(…)?
There was a problem hiding this comment.
I use .one() for the ready event.
There was a problem hiding this comment.
Have you tested this and does it work?
This doesn’t seem to work: http://jsfiddle.net/mathias/tMDrZ/ Any ideas? Perhaps we could use something like this: http://jsfiddle.net/tMDrZ/1/
There was a problem hiding this comment.
You're right, it doesn't work. I thought about it, and ultimately we can assume that by the time .placeholder is called, domready is already fired.
|
@cheeaun No, no notifications for that :( Thanks for the ping; I’ve left some comments. |
| // Clear placeholder values upon page reload | ||
| $(window).one('unload.placeholder', function() { | ||
| $('.' + $.fn.placeholder.className).val(''); | ||
| }); |
There was a problem hiding this comment.
We could chain these together: $(window).ready(…).one(…);
Add `initialized` variable to prevent the events binded more than once.
| @@ -14,32 +14,37 @@ | |||
|
|
|||
| } else { | |||
|
|
|||
| var initialized = false; | |||
There was a problem hiding this comment.
Can you move this line to the top of the scope please?
|
Changes have been committed. |
|
Looks good to me. Did you test this? |
|
Hey all, I came here by way of #175. I'm sure there are plenty of bootstrap users out there in addition to me, is there anything I can do to move this forward? |
|
I can take a look at this. The idea to have custom class name is great. This PR has fallen behind, can you bring it back to date? |
|
This feature is already supported. Closing this. |
Fix for issue #38 . I left out the minified version in the commit just because.