Skip to content
This repository has been archived by the owner on May 29, 2019. It is now read-only.

feat(buttons): add checkbox and radio buttons #152

Closed
wants to merge 1 commit into from

Conversation

pkozlowski-opensource
Copy link
Member

Closes #53

Hopefully this one is not very controversial / easy to review.

@joshdmiller
Copy link
Contributor

@pkozlowski-opensource Awesome.

This is why I love AngularJS - the tests are longer the directives!

@SidhNor
Copy link
Contributor

SidhNor commented Feb 18, 2013

@pkozlowski-opensource looks great to me. Really simple and clean

@ajoslin
Copy link
Contributor

ajoslin commented Feb 18, 2013

Wow, super simple. Nice @pkozlowski-opensource ! 👍

94 lines of tests and 73 lines of code. As @joshdmiller said, AngularJS wins 😄

Only one thing - can we make the event for changing the buttons a config value, defaulting to click?

eg element.bind(buttonConfig.toggleEvent, function() { - so on mobile we can replace click with a tap event :-)

I just can't think of a good name for the config value.. changeEvent, toggleEvent, maybe even just clickEvent?

@pkozlowski-opensource
Copy link
Member Author

Yeh, AngularJS rules the World :-)

Sure, let's make the events configurable, toggleEvent sounds nice if you ask me.

@petebacondarwin
Copy link
Member

actionEvent although I think toggleEvent is better.

@petebacondarwin
Copy link
Member

Actually, is this removal of hard-coded "button" events something that should be dealt with throughout?
Igor was talking about best practices with directives recently and he suggested that click/tap was abstracted to some action event - even so far as to use ng-action rather than ng-click, etc.

@pkozlowski-opensource
Copy link
Member Author

Landed as 571ccf4

For now added a config option for the toggleEvent as pointed out by @ajoslin but I agree with @petebacondarwin that we should start thinking about encapsulating it in a more generic way.

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.

Add a directive for the button toggle
5 participants