feat(button): add new button directive

[pkozlowski-opensource]

Proposal for #53

[pkozlowski-opensource]

While playing with this directive I kept asking myself 2 questions:

• should we be adding the btn class automatically to each and every button? Maybe it is going too far? At the same time it is nice since we don't to add the btn class over and over again.
• what should be name of the toggle attribute? Isn't the toggle too generic? Maybe it should be btn-toggle?

Thoughts?

[ajoslin]

You could just watch exprGetter, since angular will be $parsing it internally anyway if you provide an expression.

[pkozlowski-opensource]

Not sure I got you here...

[ajoslin]

I don't think i like the automatic class=btn, it's too magical. Adding class=btn manually is no problem for me.

And we should make it an attribute directive of name btn-toggle. Lots of people use anchor or div elements for their buttons.

[ajoslin]

I just thought of this, but we should be keeping click events out of our code. On mobile, they are bad (need a tap event). We should keep the events in template. Although here I'm not sure how to make the event mobile configurable... A template for this directive would be too much.

[pkozlowski-opensource]

Yeh, adding event in a directive is bad... although I thought that it will react to tap events as well, need to test.

What worries me more is is the fact that the class name (active) is hard-coded here.
I like the idea of this directive less and less :-)

Maybe we really should just do a generic class-toggle and be done with it? WDYT?

[pkozlowski-opensource]

Thnx @ajoslin for looking into it.

I agree with getting rid of the automatic btn, wanted to see how it goes but yeh, agree with the "too magical".

Maybe we should just do the class-toggle directive, an attribute directive along those lines:

class-toggle="classname for expression" ?

Or maybe we should abandon the idea of such a simple directive altogether and simply have demos for cases where a dedicated directive is too much?

[joshdmiller]

@pkozlowski-opensource As this is such a simple directive, I am more in favor it not existing and just incorporating example use cases, I think. For most buttons, the use case is too simple:

<button class="btn" ng-click="doStuff()">Click Me!</button>

As for the toggling, which seems to be all the directive does, I agree with making a class-toggle attribute directive, as you suggested, that supports multiple class-expression combinations. I can think of many scenarios beyond buttons where this would also be useful.

[pkozlowski-opensource]

@joshdmiller Yes, I agree that the directive as submitted in this PR doesn't make much sense. I will just keep it open to continue the discussion but for sure it shouldn't be merged.

But yes, a class-toggling directive might make sense. Could you work with me on the syntax?

My current proposal was: class-toggle="myclass for expression" where:

• myclass - a class to toggle (it could be probably an expression as well)
• expression - expression to evaluate. If it evaluates to true classes should be added, otherwise removed.
  But this directive should also register a click event to toggle those classes. If not it doesn't bring anything as one could simply do:

<button ng-class="active: expression" ng-click="expression = !expression">Click me</button>

I don't know, maybe there is nothing to encapsulate here?

[ajoslin]

Yeah, I don't think it's quite worth a directive.

Sent from my iPhone

On Jan 5, 2013, at 3:02 PM, Pawel Kozlowski notifications@github.com wrote:

@joshdmiller Yes, I agree that the directive as submitted in this PR doesn't make much sense. I will just keep it open to continue the discussion but for sure it shouldn't be merged.

But yes, a class-toggling directive might make sense. Could you work with me on the syntax?

My current proposal was: class-toggle="myclass for expression" where:

myclass - a class to toggle (it could be probably an expression as well)
expression - expression to evaluate. If it evaluates to true classes should be added, otherwise removed. But this directive should also register a click event to toggle those classes. If not it doesn't bring anything as one could simply do:
Click me

I don't know, maybe there is nothing to encapsulate here?

—
Reply to this email directly or view it on GitHub.

[joshdmiller]

After a little more thought, I agree: it's not worth it.

That said, I do think having some examples like this one in the docs would be very beneficial.

[pkozlowski-opensource]

OK, so clearly this PR doesn't make much sense :-)

Closing this one. The issue for buttons is still open, we can decide if we simply want to add some examples or think of another directive.

[fingermark]

Agreed. -- new angular user