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.

Sign up for GitHub

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

Legend scroll fix #2426

Merged
merged 6 commits into from
Mar 1, 2018
Merged

Legend scroll fix #2426

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f75f5ba
fix #2387 - prevent legend scrolling from scrollBox drag or right click
alexcjohnson Feb 28, 2018
ec42784
grab the legend scrollbar at the cursor, not at its middle
alexcjohnson Feb 28, 2018
246b77f
make legend scrollbar scale to the visible fraction
alexcjohnson Feb 28, 2018
569b378
fix #808 - update scrollBar when changing other legend attributes
alexcjohnson Mar 1, 2018
5660bf5
:hocho: `call` when not in a chain
alexcjohnson Mar 1, 2018
f2d8cfd
turn `scrollBoxY` into a positive number
alexcjohnson Mar 1, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 10 additions & 11 deletions src/components/legend/draw.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,8 +243,7 @@ module.exports = function draw(gd) {
var scrollBoxYMax = opts._height - legendHeight;
var scrollRatio = scrollBarYMax / scrollBoxYMax;

// scrollBoxY is 0 or a negative number
var scrollBoxY = Math.max(opts._scrollY || 0, -scrollBoxYMax);
var scrollBoxY = Math.min(opts._scrollY || 0, scrollBoxYMax);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!


// increase the background and clip-path width
// by the scrollbar width and margin
Expand All @@ -265,7 +264,7 @@ module.exports = function draw(gd) {
constants.scrollBarMargin,
height: legendHeight - 2 * opts.borderwidth,
x: opts.borderwidth,
y: opts.borderwidth - scrollBoxY
y: opts.borderwidth + scrollBoxY
});

Drawing.setClipUrl(scrollBox, clipId);
Expand All @@ -274,11 +273,11 @@ module.exports = function draw(gd) {

legend.on('wheel', function() {
scrollBoxY = Lib.constrain(
opts._scrollY -
opts._scrollY +
d3.event.deltaY / scrollBarYMax * scrollBoxYMax,
-scrollBoxYMax, 0);
0, scrollBoxYMax);
scrollHandler(scrollBoxY, scrollBarHeight, scrollRatio);
if(scrollBoxY !== 0 && scrollBoxY !== -scrollBoxYMax) {
if(scrollBoxY !== 0 && scrollBoxY !== scrollBoxYMax) {
d3.event.preventDefault();
}
});
Expand All @@ -295,8 +294,8 @@ module.exports = function draw(gd) {
if(e.buttons === 2 || e.ctrlKey) return;

scrollBoxY = Lib.constrain(
(eventY0 - e.clientY) / scrollRatio + scrollBoxY0,
-scrollBoxYMax, 0);
(e.clientY - eventY0) / scrollRatio + scrollBoxY0,
0, scrollBoxYMax);
scrollHandler(scrollBoxY, scrollBarHeight, scrollRatio);
});

Expand All @@ -306,17 +305,17 @@ module.exports = function draw(gd) {

function scrollHandler(scrollBoxY, scrollBarHeight, scrollRatio) {
opts._scrollY = gd._fullLayout.legend._scrollY = scrollBoxY;
Drawing.setTranslate(scrollBox, 0, scrollBoxY);
Drawing.setTranslate(scrollBox, 0, -scrollBoxY);

Drawing.setRect(
scrollBar,
legendWidth,
constants.scrollBarMargin - scrollBoxY * scrollRatio,
constants.scrollBarMargin + scrollBoxY * scrollRatio,
constants.scrollBarWidth,
scrollBarHeight
);
clipPath.select('rect').attr({
y: opts.borderwidth - scrollBoxY
y: opts.borderwidth + scrollBoxY
});
}

Expand Down
26 changes: 13 additions & 13 deletions test/jasmine/tests/legend_scroll_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,15 +99,15 @@ describe('The legend', function() {
2 * constants.scrollBarMargin;
var initialDataScroll = getScroll(gd);
var wheelDeltaY = 100;
var finalDataScroll = Lib.constrain(initialDataScroll -
var finalDataScroll = Lib.constrain(initialDataScroll +
wheelDeltaY / scrollBarYMax * scrollBoxYMax,
-scrollBoxYMax, 0);
0, scrollBoxYMax);

legend.dispatchEvent(scrollTo(wheelDeltaY));

expect(getScroll(gd)).toBe(finalDataScroll);
expect(scrollBox.getAttribute('transform')).toBe(
'translate(0, ' + finalDataScroll + ')');
'translate(0, ' + -finalDataScroll + ')');
});

function dragScroll(element, rightClick) {
Expand All @@ -120,9 +120,9 @@ describe('The legend', function() {
2 * constants.scrollBarMargin;
var initialDataScroll = getScroll(gd);
var dy = 50;
var finalDataScroll = Lib.constrain(initialDataScroll -
var finalDataScroll = Lib.constrain(initialDataScroll +
dy / scrollBarYMax * scrollBoxYMax,
-scrollBoxYMax, 0);
0, scrollBoxYMax);

var y0 = scrollBarBB.top + scrollBarBB.height / 5;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

notice that I moved the drag point away from the center of the scrollBar, thereby testing that it drags from where you grab it rather than snapping to the center.

var y1 = y0 + dy;
Expand Down Expand Up @@ -152,7 +152,7 @@ describe('The legend', function() {
var dataScroll = getScroll(gd);
expect(dataScroll).toBeCloseTo(finalDataScroll, 3);
expect(scrollBox.getAttribute('transform')).toBe(
'translate(0, ' + dataScroll + ')');
'translate(0, ' + -dataScroll + ')');
});

it('should not scroll on dragging the scrollbox', function() {
Expand All @@ -162,7 +162,7 @@ describe('The legend', function() {
var dataScroll = getScroll(gd);
expect(dataScroll).not.toBeCloseTo(finalDataScroll, 3);
expect(scrollBox.getAttribute('transform')).toBe(
'translate(0, ' + dataScroll + ')');
'translate(0, ' + -dataScroll + ')');
});

it('should not scroll on dragging the scrollbar with a right click', function() {
Expand All @@ -172,7 +172,7 @@ describe('The legend', function() {
var dataScroll = getScroll(gd);
expect(dataScroll).not.toBeCloseTo(finalDataScroll, 3);
expect(scrollBox.getAttribute('transform')).toBe(
'translate(0, ' + dataScroll + ')');
'translate(0, ' + -dataScroll + ')');
});

it('removes scroll bar and handlers when switching to horizontal', function(done) {
Expand Down Expand Up @@ -214,15 +214,15 @@ describe('The legend', function() {
expect(scrollBarHeight1).toBeGreaterThan(scrollBarHeight);

// we haven't quite removed the scrollbar, but we should have clipped the scroll value
return Plotly.deleteTraces(gd, [0, 1, 2, 3, 4, 5, 6, 7]);
return Plotly.deleteTraces(gd, [0, 1, 2, 3, 4, 5, 6]);
})
.then(function() {
expect(getScroll(gd)).toBeGreaterThan(dataScroll + 1);
expect(getScroll(gd)).toBeLessThan(dataScroll - 1);
var scrollBarHeight2 = getScrollBar().getBoundingClientRect().height;
expect(scrollBarHeight2).toBeGreaterThan(scrollBarHeight1);

// now no more scrollBar
return Plotly.deleteTraces(gd, [0, 1]);
return Plotly.deleteTraces(gd, [0, 1, 2]);
})
.then(function() {
expect(hasScrollBar()).toBe(false);
Expand Down Expand Up @@ -251,7 +251,7 @@ describe('The legend', function() {
expect(+toggle.parentNode.style.opacity).toBeLessThan(1);
expect(getScroll(gd)).toBe(dataScroll);
expect(scrollBox.getAttribute('transform')).toBe(
'translate(0, ' + dataScroll + ')');
'translate(0, ' + -dataScroll + ')');
done();
}, DBLCLICKDELAY * 2);
});
Expand Down Expand Up @@ -289,7 +289,7 @@ describe('The legend', function() {
expect(+toggle.parentNode.style.opacity).toBeLessThan(1);
expect(getScroll(gd)).toBe(dataScroll);
expect(scrollBox.getAttribute('transform')).toBe(
'translate(0, ' + dataScroll + ')');
'translate(0, ' + -dataScroll + ')');
expect(scrollBar.getAttribute('width')).toBeGreaterThan(0);
expect(scrollBar.getAttribute('height')).toBeGreaterThan(0);
done();
Expand Down