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

Implement limits #115

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Implement limits #115

wants to merge 11 commits into from

Conversation

nicofrand
Copy link

See #85

@ghusse
Copy link
Owner

ghusse commented Apr 26, 2013

I created a branch named "limits" in order to propose one modification in your code:

https://github.com/ghusse/jQRangeSlider/tree/limits

Take a look at it, and if it seems good to you, merge it in your branch.

I removed the option limits in the bar, removed the code in _mouseDrag, and let handles constrain values. The bar just tries to keep the same distance between both handles.

Nicolas Frandeboeuf added 2 commits April 26, 2013 14:37
Conflicts:
	jQRangeSliderBar.js
@nicofrand
Copy link
Author

Thanks. I merged it with my branch and also updated the other things you reported.
The handles are still bouncing in Chrome but besides that, it works juste fine to me.

@ghusse
Copy link
Owner

ghusse commented Apr 26, 2013

We need to investigate on this bouncing issue.

We also need to make the build to pass, and to add unit tests to verify that:

  • Option is correctly set when passed in constructor (with undefined, false, define values for each property)
  • Option is correctly modified after construction (with same values)
  • When activated, user cannot slide the bar outside the limits
  • When activated, user cannot slide handles outside the limits

@nicofrand
Copy link
Author

I still need to investigate further but, for the bouncing issue, this seems the root of the issue is that sometimes the offset values in cache are rounded integers instead of floats.
For example, I expect "308.5263565" and the value in cache is "308".

@nicofrand
Copy link
Author

jQuery offset methods always returns integers (in Chrome at least) whereas we sometimes set the position from a position value based on event.pageX, which is a float.

Rounding all the position fixes the issue for me.
In jQrangeSliderDraggable, _applyPosition :

_applyPosition: function(position){
            position = position >> 0;
            var offset = {
                top: this.cache.offset.top,
                left: position
            };

            this.element.offset({left:position});

            this.cache.offset = offset;
        }

Would that be a problem @ghusse ?
If this is not, the positions should always be rounded of course (to obtain correct values at least).

@ghusse
Copy link
Owner

ghusse commented May 2, 2013

I'm ok with that. Just a question, do you know how it react when we call offset with a float value ? How does it round the value ? Is it a mathematical rounding, a truncation ?

@nicofrand
Copy link
Author

I did it to follow your code (in jQDateRangeSlider#_setOption and #values).
I will update the code and commit it today.

@nicofrand
Copy link
Author

OK. So, this might be not optimal but I found a solution. The issue came some, I think so at least, a matter of js handling the numbers : there were some really really slight difference between two positions (whereas it should not) like between "196" and "195.999999999956".
I modified jQRangeSliderHandle#position to ignore those minor differences (0.005 error margin), that should not cause any other issues…

@ghusse
Copy link
Owner

ghusse commented May 14, 2013

Ok, great. It's not JS specific, it's a known limitation of float numbers binary encoding.

@nicofrand
Copy link
Author

Argh, once again, I did not test enough. This breaks the bar dragging (partially). I guess I did not put it in the good place.

PS: indeed, this is not JS specific.

@nicofrand
Copy link
Author

There are some things I do not understand.

For example, I logged the positions I received in several methods. This is what happens when the bar is not expected to move (and the values to change) :

Draggable drag: (old) 195.9953930144298 (new)  197 jQRangeSliderDraggable.js:77
Constraint position (before):  197 jQRangeSliderBar.js:337
Draggable constraint position:  197 jQRangeSliderDraggable.js:105
Pos before constraint (handle):  197 jQRangeSliderHandle.js:151
Pos after constraint (handle):  197 jQRangeSliderHandle.js:158
set values 1360261205072 jQRangeSliderHandle.js:188
Pos before constraint (handle):  1593 jQRangeSliderHandle.js:151
Pos after constraint (handle):  1592.0046069855703 jQRangeSliderHandle.js:158
Pos before constraint (handle):  196.00460698557026 jQRangeSliderHandle.js:151
Pos after constraint (handle):  196.00460698557026 jQRangeSliderHandle.js:158
set values 1360255373976 jQRangeSliderHandle.js:188
Constraint position (after):  196.00460698557026 jQRangeSliderBar.js:351

In jQRangeSliderDraggable.js:77 the old position from the first line is this.cache.offset.left.
The position in the last line (the one returned) is correct since it is the same (almost, due to float precision and calculating "errors"). But during the process, the values were set two times.

@ghusse
Copy link
Owner

ghusse commented May 16, 2013

What kind of interaction are you doing in this case ?

@nicofrand
Copy link
Author

I am dragging the bar. Each handle of the bar is stuck to the limits, so the bar is not supposed to move (which is almost the case since) and, the values are not supposed to change (since the bar did not move).

@ghusse
Copy link
Owner

ghusse commented May 16, 2013

It seems that constraintValues is not returning exactly the same value + I cannot see a test in the code verifying that before and after values are different before setting new positions.

Where position is constrained: https://github.com/ghusse/jQRangeSlider/blob/master/jQRangeSliderDraggable.js#L73

Then applied: https://github.com/ghusse/jQRangeSlider/blob/master/jQRangeSliderDraggable.js#L100

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.

2 participants