A bunch of fixes and additions #253

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
Contributor

csuwildcat commented Jun 21, 2011

No description provided.

Contributor

fat commented Jun 21, 2011

commit of a bunch of untested changes with incorrect whitespace?

Owner

arian commented Jun 21, 2011

What does it fix?

Owner

csuwildcat commented on e4c6d1c Jun 22, 2011

Slider.js - code fixes the issue that was raised again today for the 1000th time (in office hours none the less) where the knob doesn't snap to the end of the slider element if the division leaves a remainder - devs shouldn't have to worry about making their layout certain sizes to fit our bugs. It also allows for Slider.Extras, which is just pimp.

Fx.Scroll - fixes the fact that Fx.Scroll doesn't account for the current amount the element is already scrolled, this causes the code to transition to the wrong place.

Request.JSONP - an enhancement to allow the options of the particular send() instance to be passed to the success function. This is important because the options change per call so not tying it to the instance success means you can be sure they are the right ones as the return is async.

Owner

arian commented Jun 22, 2011

Thanks for the explanation. Unfortunately I don't have time to pull this now. Providing some tests (I assume you tested your changes locally) would help. Preferably as specs and tests in the Tests folder of course.

Owner

arian commented Jun 22, 2011

Your sliders changes would fix #252 right?

@arian arian commented on the diff Jul 31, 2011

Source/Drag/Slider.js
@@ -36,6 +36,12 @@ var Slider = new Class({
onTick: function(position){
this.setKnobPosition(position);
},
+ onStart: function(){
+ this.draggedKnob();
@arian

arian Jul 31, 2011

Owner

Wouldn't this break when you set the events with the options object during the initialization.
So isn't it better to use this in the initialize method?

this.addEvent('start', this.draggedKnob.bind(this), true);

@arian arian commented on the diff Jul 31, 2011

Source/Drag/Slider.js
- modifiers: modifiers,
- onDrag: this.draggedKnob,
- onStart: this.draggedKnob,
- onBeforeStart: (function(){
- this.isDragging = true;
- }).bind(this),
- onCancel: function(){
- this.isDragging = false;
- }.bind(this),
- onComplete: function(){
- this.isDragging = false;
- this.draggedKnob();
- this.end();
- }.bind(this)
+ var self = this,
+ dragOptions = {
@arian

arian Jul 31, 2011

Owner

keep the var here, and don't extra indent. Makes it hard to review as well.

@arian arian commented on the diff Jul 31, 2011

Source/Fx/Fx.Scroll.js
['x', 'y'].each(function(axis){
if (axes.contains(axis)){
- if (edge[axis] > scroll[axis] + containerSize[axis]) to[axis] = edge[axis] - containerSize[axis];
- if (position[axis] < scroll[axis]) to[axis] = position[axis];
+ if (edge[axis] > containerSize[axis]) to[axis] = (edge[axis] - containerSize[axis]) + scroll[axis];
@arian

arian Jul 31, 2011

Owner

Doesn't this apply for that other method as well?

@arian arian commented on the diff Jul 31, 2011

Source/Request/Request.JSONP.js
@@ -83,7 +83,7 @@ Request.JSONP = new Class({
if (src.length > 2083) this.fireEvent('error', src);
Request.JSONP.request_map['request_' + index] = function(){
- this.success(arguments, index);
+ this.success(Array.from(arguments).append(options), index);
@arian

arian Jul 31, 2011

Owner

I see that it's not easy to access the options like url and data in the success method, however we don't have this in Request as well, and I don't want to add extra stuff that is inconsistent across the framework (so with core)

Owner

arian commented Jul 31, 2011

Isn't the Fx.Scroll fix the same as #239

@SergioCrisostomo SergioCrisostomo added a commit to SergioCrisostomo/mootools-more that referenced this pull request May 3, 2014

@SergioCrisostomo SergioCrisostomo Fix "snap" to final value
Imported a idea from @csuwildcat 's PR #253
fixes #252
349e30b

@SergioCrisostomo SergioCrisostomo added a commit that referenced this pull request May 6, 2014

@SergioCrisostomo SergioCrisostomo Fix "snap" to final value
Imported a idea from @csuwildcat 's PR #253
fixes #252
62f78bf
Member

SergioCrisostomo commented Oct 25, 2015

I used some ideas from this PR in earlier fixes. Thanks :)
Closing this now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment