Skip to content

feat(rzSliderOptions): Change rzSliderOptions to use expression binding #266

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

rabbitfang
Copy link
Contributor

Switch from using two-way binding for rzSliderOptions to using expression binding. Using expression
binding enables more complex use cases such as using the return value of a function as an options
object.

Closes #265.

@rabbitfang
Copy link
Contributor Author

If more detailed tests are needed, I can add them. I wasn't quite sure what I should test. I added the optionsExpression option for the test helper for setting the expression that is used for directive (defaults to slider.options).

@rabbitfang
Copy link
Contributor Author

Example JSfiddle for why this is useful: http://jsfiddle.net/xd2zggvb/

Switch from using two-way binding for rzSliderOptions to using expression binding. Using expression
binding enables more complex use cases such as using the return value of a function as an options
object.

Closes angular-slider#265.
@rabbitfang rabbitfang force-pushed the options-expression-binding branch from 62dcad7 to 018d93e Compare February 16, 2016 15:58
@codecov-io
Copy link

Current coverage is 100.00%

Merging #266 into master will not affect coverage as of 9dc5f25

@@            master    #266   diff @@
======================================
  Files            1       1       
  Stmts          641     645     +4
  Branches         0       0       
  Methods          0       0       
======================================
+ Hit            641     645     +4
  Partial          0       0       
  Missed           0       0       

Review entire Coverage Diff as of 9dc5f25

Powered by Codecov. Updated on successful CI builds.

@ValentinH
Copy link
Member

I have to admit that at first I didn't really like it and it feels weird to me, but it works without creating breaking changes (as far as I have tested for now).
I'm just curious of where have you seen such a way to pass parameters to a directive? I was thinking that expression bindings where only good for calling callbacks from a directive...

@rabbitfang
Copy link
Contributor Author

I personally use expression binding when there is no need for the directive to make modifications to a value (and where attribute binding wouldn't make sense). Expression binding allows the greatest freedom for the consuming developer. One common example for when AngularJS itself uses expression binding over two-way binding is the ngShow, ngHide and ngIf directives. By using expression binding there, I can use something such as ng-if="!shouldHide". It also allows filters to be in the parameter.

For rzslider, I use the return value of a function that dynamically generates the slider's configuration based on various parameters that it pulls from various locations. Even though the $watch uses angular.equals comparison to check for updates, the two way binding only checks for identity. If the function returns a new object on every call, a infinite digest loop is created.

@ValentinH
Copy link
Member

Well, looks good to me! Thanks :)

ValentinH added a commit that referenced this pull request Feb 17, 2016
feat(rzSliderOptions): Change rzSliderOptions to use expression binding
@ValentinH ValentinH merged commit 77a0aeb into angular-slider:master Feb 17, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants