Skip to content
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

Slide binding. It now accepts onChange #3

Merged
merged 5 commits into from
May 26, 2015
Merged

Slide binding. It now accepts onChange #3

merged 5 commits into from
May 26, 2015

Conversation

drochag
Copy link
Collaborator

@drochag drochag commented May 22, 2015

4 mayor changes:

  • onClick acceptance through latest SimpleSlider dist files.
  • slider double binding so any outer scope can access directly to the SimpleSlider API
  • slider's slide double binding so more natural angular digest happens
  • directive now waits until dynamic children are loaded

@@ -32,7 +32,7 @@
<option value="3">Slide 4</option>
</select>

<simple-slider style="width:612px; height:612px" auto-play="false" change="{{actualslide}}">
<simple-slider simple-slider-slide="actualslide" style="width:612px; height:612px" auto-play="false">
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to see a better attribute name for the slide attribute, actual-slide or current-slide are definitely better

@ruyadorno
Copy link
Owner

Many thanks @danmmx ! 😄

These are awesome ideas and fixes! I'm taking the time to comment on a few things and let's get it merged :)

@drochag
Copy link
Collaborator Author

drochag commented May 25, 2015

Good to know @ruyadorno ^_^ let me know when you finished commenting so we don't overlap changes + comments.

scope: {
onChange: '&',
slide: '=?simpleSliderSlide',
slider: '=?simpleSlider'
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to confess that my angular skills are a bit rusty these days, so help me out with this one.

Is it really a good idea to expose the SimpleSlider object as a simple-slider attribute?

My original idea was to hide everything behind the directive implementation and the consumer would just play with the attributes. I don't really know what are the trade-offs involved here. Since you're already exposing onchange and the simple-slider-slide attribute it seems that it would make sense to keep the consumers away from the simpleSlider instance, have you give it some thought? I would love to have some feedback 😊

@ruyadorno
Copy link
Owner

Looking awesome 😄 I'm done with reviews, just the watcher var renaming and we're good to go

@drochag
Copy link
Collaborator Author

drochag commented May 26, 2015

I think it's ready @ruyadorno 👍

@ruyadorno
Copy link
Owner

👍 LGTM

ruyadorno added a commit that referenced this pull request May 26, 2015
Slide binding. It now accepts onChange
@ruyadorno ruyadorno merged commit e06f965 into ruyadorno:master May 26, 2015
@drochag drochag mentioned this pull request Aug 16, 2015
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.

None yet

2 participants