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

feat(rating): add rating directive #341

Closed
wants to merge 2 commits into from
Closed

Conversation

bekos
Copy link
Contributor

@bekos bekos commented Apr 19, 2013

Based on #337.

@pkozlowski-opensource
Copy link
Member

LGTM after having a short look. +1 for merging it before 0.3.0 provided that is passes tests in all browsers.

@pkozlowski-opensource
Copy link
Member

@bekos Had another quick look, added some minor points in the comments

But I still wonder if we shouldn't be using NgModelController here so people would be writing:
<rating ng-model="rate" max="10" readonly="isReadonly"></rating>

The advantage here would be that people could add custom things to a pipeline (data type conversion etc.). Beside the code would be smaller, no?

Normally I wouldn't care that much as we can always change it later but it would mean changing attribute name. Up to you.

Great work BTW, very clean PR and a nice addition to the set of existing directives!

@bekos
Copy link
Contributor Author

bekos commented Apr 20, 2013

I don't prefer ng-model because someone can also do something like this

<rating value="4" readonly="true"></rating>

but if you think otherwise I can change it. No strong opinion here.

@pkozlowski-opensource
Copy link
Member

@bekos I see your point about specifying a non-assignable expression. I don't have strong feelings either way, will leave the final decision in your hands. Unless others want to weight here.

@ajoslin
Copy link
Contributor

ajoslin commented Apr 22, 2013

👍 Looks good. :-)

@bekos I think you are right, that readonly is a valid use case.

 * Remove ng-class escaping from template.
 * Use function to test star's state.
@bekos
Copy link
Contributor Author

bekos commented Apr 22, 2013

Comments are fixed.
I will keep thinks as they are, since and @ajoslin agrees ( I'm not used to this :-) )

@pkozlowski-opensource Thank you very much for the comments :-)

@pkozlowski-opensource
Copy link
Member

@bekos squashed commits and landed it as 6b5e636.

Great work!

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.

3 participants