Skip to content

ensure newRng stays in bounds - #4448

Merged
archmoj merged 3 commits into
plotly:masterfrom
jay-bis:fix-rangeslider-oob-bug
Mar 14, 2023
Merged

ensure newRng stays in bounds#4448
archmoj merged 3 commits into
plotly:masterfrom
jay-bis:fix-rangeslider-oob-bug

Conversation

@jay-bis

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

Copy link
Copy Markdown

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.

@etpinard

etpinard commented Dec 30, 2019

Copy link
Copy Markdown
Contributor

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

@jay-bis

jay-bis commented Dec 30, 2019

Copy link
Copy Markdown
Author

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

etpinard commented Dec 30, 2019

Copy link
Copy Markdown
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

plotly.js/test/jasmine/tests/range_slider_test.js

Lines 152 to 202 in e778644

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

jay-bis commented Dec 30, 2019

Copy link
Copy Markdown
Author

For sure! I'll get started on that.

@klausbreyer

klausbreyer commented Dec 14, 2022

Copy link
Copy Markdown

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

@YMNNs

YMNNs commented Mar 14, 2023

Copy link
Copy Markdown

@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

archmoj commented Mar 14, 2023

Copy link
Copy Markdown
Contributor

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

@archmoj
archmoj merged commit 2f0bf80 into plotly:master Mar 14, 2023
@brendanmatkin brendanmatkin mentioned this pull request Apr 17, 2023
Open
@markchagers

markchagers commented Feb 28, 2024

Copy link
Copy Markdown

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

Reviewers

No reviews

Assignees

No one assigned

Labels

bug something broken community community contribution

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@jay-bis @etpinard @klausbreyer @YMNNs @archmoj @markchagers