[Fix] Add optional param in play() function - Githubissues

Rise-Vision / auto-scroll

Auto-scroll jQuery Plugin

Other

1 stars 2 forks source link

[Fix] Add optional param in play() function #16

Closed stulees closed 9 years ago

stulees commented 9 years ago

In the scenario that a pause() function was called after a Draggable.onRelease occurred, there is no delated tween call anymore. So when a further play() is called, the scrolling jumps to the beginning again.
Now the play() function accepts a progress boolean so that if provided, tween.progress() will be called with appropriate calculation and then tween.play()

stulees commented 9 years ago

@donnapep Please review. FYI, I couldn't see a better way to handle this fix when considering our existing Widgets and also what I need to do for IU. Thanks

donnapep commented 9 years ago

@stulees Yeah, that feels pretty hacky. At a minimum, I think the calculateProgress callback should be moved to its own function so that the code isn't duplicated.

Is it possible to just set a boolean when a click event happens and then check that boolean in pause and stop and only kill the call to calculateProgress in that situation? Would that work?

stulees commented 9 years ago

@donnapep Where do you feel the calculateProgress function should be best placed for reuse?

Are you suggesting to set a boolean in the plugin via a public function? Or providing a boolean when calling the plugin pause() or stop()?

donnapep commented 9 years ago

@stulees I think right below canScroll is the best place since it will be private to the plugin.

What event are you using to trap a click? Is it one of the plugin's events?

stulees commented 9 years ago

@donnapep I've changed this to now accept a boolean param in pause and stop to account for a "freeze". calculateProgress is now a function onto itself and the variables used are back being local to the plugin init function.

donnapep commented 9 years ago

@stulees As discussed in hangout, I was thinking that adding an onclick event to Draggable would work for the popup card and that is where the boolean would be set to indicate that the click event had happened. And then that's the boolean that could be checked in pause and stop. I think in future, that's how it should be changed to work and hammer.js is a temporary solution.

I also think this repo needs to be versioned / Bower-enabled, because if we were to go ahead and make the change as per the above, it would break the IU Presentation. It needs to have a local copy and not have a direct link to the Amazon version.

donnapep commented 9 years ago

@stulees Actually, the added parameters will break our existing Widgets, right?

stulees commented 9 years ago

@donnapep The added parameters should not break our existing Widgets as they are an optional boolean. So the conditional check if (freeze) will be false if the param is omitted since undefined will be falsy.

donnapep commented 9 years ago

@stulees I've been asking myself the question of would I approve this PR if there was not a time crunch involved, and the answer I arrived at is that I wouldn't.

This feels like too much of a hack. We're handling a click event outside of the plugin, but then adding logic to the plugin to try to deal with undesirable behaviour resulting from that outside click. This is technical debt. The click event should be handled cleanly inside of the plugin, and should eliminate the need for passing parameters to functions.

I see that this repo is already bower-enabled. I think we should bump the version with this PR as well.

Call me and we can discuss further if you like.