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

ensure newRng stays in bounds #4448

Merged
merged 3 commits into from
Mar 14, 2023
Merged

Conversation

jay-bis
Copy link

@jay-bis jay-bis commented Dec 28, 2019

Created a few checks to make sure the slider's range doesn't go out of its normal bounds. This is an attempt to fix the bug outlined by this gif.

I'm not sure what the preferred behavior would be when you do drag the slider to the max/min bounds, but what this solution does is snap the slider back to the graph's corresponding max/min x value when the slider is dragged past the max/min.

Jack Biscupski added 2 commits December 28, 2019 12:33

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@etpinard
Copy link
Contributor

Hi @jay-bis - are you attempting to fix the same bug as presented in #4417 (comment) here?

@jay-bis
Copy link
Author

jay-bis commented Dec 30, 2019

Yes, I am! My first commit caused some of the tests to fail and was still a bit buggy; my newest commit passed all the tests and seems to work a bit more smoothly, and gets rid of the bug as far as I've tested it.

@etpinard
Copy link
Contributor

Ok great - thanks!

Your second commit looks good. Would you be interested in trying to write a few test cases to lock down your fix?

If so, the new tests should be similar to

it('should react to moving the slidebox left to right', function(done) {
var start = 250;
var end = 300;
var diff = end - start;
var dataMinStart;
plotMock().then(function() {
dataMinStart = gd._fullLayout.xaxis.rangeslider.d2p(0);
expect(gd.layout.xaxis.range).toBeCloseToArray([0, 49]);
return slide(start, sliderY, end, sliderY);
})
.then(function() {
var maskMin = getRangeSliderChild(2);
var handleMin = getRangeSliderChild(5);
expect(gd.layout.xaxis.range).toBeCloseToArray([3.96, 49], -0.5);
expect(+maskMin.getAttribute('width')).toBeCloseTo(String(diff));
testTranslate1D(handleMin, dataMinStart + diff - 3);
})
.catch(failTest)
.then(done);
});
it('should react to moving the slidebox right to left', function(done) {
var start = 300;
var end = 250;
var diff = end - start;
var dataMaxStart;
plotMock().then(function() {
dataMaxStart = gd._fullLayout.xaxis.rangeslider.d2p(49);
expect(gd.layout.xaxis.range).toBeCloseToArray([0, 49]);
return slide(start, sliderY, end, sliderY);
})
.then(function() {
var maskMax = getRangeSliderChild(3);
var handleMax = getRangeSliderChild(6);
expect(gd.layout.xaxis.range).toBeCloseToArray([0, 45.04], -0.5);
expect(+maskMax.getAttribute('width')).toBeCloseTo(-diff);
testTranslate1D(handleMax, dataMaxStart + diff);
})
.catch(failTest)
.then(done);
});

@etpinard etpinard added status: in progress bug something broken labels Dec 30, 2019
@jay-bis
Copy link
Author

jay-bis commented Dec 30, 2019

For sure! I'll get started on that.

Verified

This commit was signed with the committer’s verified signature.
MichiBaum Michael Baumberger
@klausbreyer
Copy link

@etpinard I just stumbled upon #4417 and wanted kindly ask if there is any chance, that this will be merged in soon?

@YMNNs
Copy link

YMNNs commented Mar 14, 2023

@archmoj It really works, please consider merging #4448.

@archmoj archmoj added status: reviewable community community contribution and removed status: has TODOs labels Mar 14, 2023
@archmoj
Copy link
Contributor

archmoj commented Mar 14, 2023

We somehow missed the updates on this PR.
Thanks very much for the PR.
💃
I am going to release a patch right after.

@markchagers
Copy link

As of today, this is still an issue even in v2.27
Any chance of this getting fixed for real?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken community community contribution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants