Feature: Introduce new form type LimitedTextarea #3
Conversation
There was a problem hiding this comment.
I think the error styling shouldn't be in the countable css @vanhoog can you please change this to a more logical place?
|
@RutgerKooijman I think it should as in this situation it is part of the bundle. Meaning it should work correctly when used independent in a different theme. Hence why I chose to include the error colour inside this bundle. |
Resources/public/css/countable.css
Outdated
| @@ -0,0 +1,10 @@ | |||
| .error { color: #900;} | |||
There was a problem hiding this comment.
@vanhoog Maybe it's better to use class names specific for this js-counter thing? .error is kind of global. Same for counter-text? Maybe specify it more on this new element?
There was a problem hiding this comment.
I could refactor it with the new Just CSS naming standard, which i'll do... I'll keep you posted! :)
There was a problem hiding this comment.
It's important to specify this correctly, maybe wrap it all in wearejust-counter class? We don't need conflicting classnames
| } | ||
| }) | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Code duplication: your doing the same things twice. Abstract the 'keyup' function so you can call it as an event listener but also call it right after init.
And instead of using an if(<CONDITION) with addClass/removeClass, you can use toggleClass('classname', ).
Also notice that it only applies to textarears, shouldnt it also work for inputs?
|
@ceesvanegmond Can't find the feature branch. What happened with his? |
Opening this PR because of a new where we could limit inputs.
See #1 for reference