Skip to content

Fix issue #384 (invisible legends when y is negative) #417

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

Merged
merged 12 commits into from
Apr 18, 2016
Merged

Fix issue #384 (invisible legends when y is negative) #417

Changes from 4 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
5ef69b3
Remove unnecessary code in repositionLegend()
n-riesco Apr 8, 2016
c68ed58
Legend.draw: Add missing `select('rect')`
n-riesco Apr 11, 2016
91c7757
Fix invisible legends with negative y
n-riesco Apr 9, 2016
cedabd3
Do not round off the legend location
n-riesco Apr 11, 2016
7f71b53
Fix computation of lowest possible legend position
n-riesco Apr 12, 2016
b747224
Fix position of legend scrollbar
n-riesco Apr 12, 2016
8baac47
Don't expand vertical margin if legend doesn't fit
n-riesco Apr 13, 2016
c96581c
Improve drag and wheel of legend scrollbars
n-riesco Apr 13, 2016
a05080d
Update legend mouse wheel test
n-riesco Apr 13, 2016
e1effce
Honour the scrollbar margin
n-riesco Apr 14, 2016
c581989
Add mock with a legend located at y -1
n-riesco Apr 14, 2016
d5e9014
Remove commented-out lines from Legend.draw()
n-riesco Apr 14, 2016
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
71 changes: 41 additions & 30 deletions src/components/legend/draw.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,27 +184,51 @@ module.exports = function draw(gd) {
if(anchorUtils.isRightAnchor(opts)) {
lx -= opts.width;
}
if(anchorUtils.isCenterAnchor(opts)) {
else if(anchorUtils.isCenterAnchor(opts)) {
lx -= opts.width / 2;
}

if(anchorUtils.isBottomAnchor(opts)) {
ly -= opts.height;
}
if(anchorUtils.isMiddleAnchor(opts)) {
else if(anchorUtils.isMiddleAnchor(opts)) {
ly -= opts.height / 2;
}

//lx = Math.round(lx);
//ly = Math.round(ly);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've commented out these two lines only to reproduce the baseline images, but I think they should be brought back.

// Make sure the legend top and bottom are visible
// (legends with a scroll bar are not allowed to stretch beyond the extended
// margins)
var lyMin = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

🐄 comma separated var declarations e.g.

var lyMin = 0,
    lyMax = fullLayout.margin.t + fu...
    // more
    legendHeight = opts.height;

var lyMax = fullLayout.margin.t + fullLayout.height + fullLayout.margin.b;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we setting this to the full height + margins? Shouldn't it just be fullLayout.height?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh! Good catch!

var legendHeight = opts.height;
var legendHeightMax = gs.h;
Copy link
Contributor

Choose a reason for hiding this comment

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

We're only using the legendHeightMax in the one condition below, so no need to define a new var here - just adds more for the GC to eat later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm using the variable names lyMin, lyMax, legendHeight and legendHeightMax for documentation purposes (implicitly, to describe the algorithm to compute legendHeight).

Mostly for this reason, I think it'd be worth keeping the variables.

Shall I remove them?



if(legendHeight > legendHeightMax) {
ly = gs.t;
legendHeight = legendHeightMax;
}
else {
if(ly > lyMax) ly = lyMax - legendHeight;

if(ly < lyMin) ly = lyMin;

legendHeight = Math.min(lyMax - ly, opts.height);
}

// Deal with scrolling
var plotHeight = fullLayout.height - fullLayout.margin.b,
scrollheight = Math.min(plotHeight - ly, opts.height),
scrollPosition = scrollBox.attr('data-scroll') ? scrollBox.attr('data-scroll') : 0;
var scrollPosition = scrollBox.attr('data-scroll') ?
Copy link
Contributor

Choose a reason for hiding this comment

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

For conditional assignments like this, we can make it a bit more concise/readable:

var scrollPosition = scrollBox.attr('data-scroll') || 0;

and use ternary operators in situations where the conditional value isn''t used except as the condition.

scrollBox.attr('data-scroll') :
0;

scrollBox.attr('transform', 'translate(0, ' + scrollPosition + ')');

bg.attr({
width: opts.width - 2 * opts.borderwidth,
height: scrollheight - 2 * opts.borderwidth,
height: legendHeight - 2 * opts.borderwidth,
x: opts.borderwidth,
y: opts.borderwidth
});
Expand All @@ -213,21 +237,21 @@ module.exports = function draw(gd) {

clipPath.select('rect').attr({
width: opts.width,
height: scrollheight,
height: legendHeight,
x: 0,
y: 0
});

legend.call(Drawing.setClipUrl, clipId);

// If scrollbar should be shown.
if(opts.height - scrollheight > 0 && !gd._context.staticPlot) {
if(opts.height - legendHeight > 0 && !gd._context.staticPlot) {

bg.attr({
width: opts.width - 2 * opts.borderwidth + constants.scrollBarWidth
});

clipPath.attr({
clipPath.select('rect').attr({
width: opts.width + constants.scrollBarWidth
});

Expand All @@ -243,21 +267,21 @@ module.exports = function draw(gd) {
scrollBox.attr('data-scroll',0);
}

scrollHandler(0,scrollheight);
scrollHandler(0,legendHeight);

legend.on('wheel',null);

legend.on('wheel', function() {
var e = d3.event;
e.preventDefault();
scrollHandler(e.deltaY / 20, scrollheight);
scrollHandler(e.deltaY / 20, legendHeight);
});

scrollBar.on('.drag',null);
scrollBox.on('.drag',null);
var drag = d3.behavior.drag()
.on('drag', function() {
scrollHandler(d3.event.dy, scrollheight);
scrollHandler(d3.event.dy, legendHeight);
});

scrollBar.call(drag);
Expand All @@ -266,12 +290,12 @@ module.exports = function draw(gd) {
}


function scrollHandler(delta, scrollheight) {
function scrollHandler(delta, legendHeight) {

var scrollBarTrack = scrollheight - constants.scrollBarHeight - 2 * constants.scrollBarMargin,
var scrollBarTrack = legendHeight - constants.scrollBarHeight - 2 * constants.scrollBarMargin,
translateY = scrollBox.attr('data-scroll'),
scrollBoxY = Lib.constrain(translateY - delta, scrollheight-opts.height, 0),
scrollBarY = -scrollBoxY / (opts.height - scrollheight) * scrollBarTrack + constants.scrollBarMargin;
scrollBoxY = Lib.constrain(translateY - delta, legendHeight-opts.height, 0),
scrollBarY = -scrollBoxY / (opts.height - legendHeight) * scrollBarTrack + constants.scrollBarMargin;

scrollBox.attr('data-scroll', scrollBoxY);
scrollBox.attr('transform', 'translate(0, ' + scrollBoxY + ')');
Expand Down Expand Up @@ -368,7 +392,6 @@ function drawTexts(context, gd, d, i, traces) {

function repositionLegend(gd, traces) {
var fullLayout = gd._fullLayout,
gs = fullLayout._size,
opts = fullLayout.legend,
borderwidth = opts.borderwidth;

Expand Down Expand Up @@ -432,39 +455,27 @@ function repositionLegend(gd, traces) {
.attr('width', (gd._context.editable ? 0 : opts.width) + 40);

// now position the legend. for both x,y the positions are recorded as
// fractions of the plot area (left, bottom = 0,0). Outside the plot
// area is allowed but position will be clipped to the page.
// values <1/3 align the low side at that fraction, 1/3-2/3 align the
// center at that fraction, >2/3 align the right at that fraction

var lx = gs.l + gs.w * opts.x,
ly = gs.t + gs.h * (1-opts.y);
// fractions of the plot area (left, bottom = 0,0).

var xanchor = 'left';
if(anchorUtils.isRightAnchor(opts)) {
lx -= opts.width;
xanchor = 'right';
}
if(anchorUtils.isCenterAnchor(opts)) {
lx -= opts.width / 2;
xanchor = 'center';
}

var yanchor = 'top';
if(anchorUtils.isBottomAnchor(opts)) {
ly -= opts.height;
yanchor = 'bottom';
}
if(anchorUtils.isMiddleAnchor(opts)) {
ly -= opts.height / 2;
yanchor = 'middle';
}

// make sure we're only getting full pixels
opts.width = Math.ceil(opts.width);
opts.height = Math.ceil(opts.height);
lx = Math.round(lx);
ly = Math.round(ly);

// lastly check if the margin auto-expand has changed
Plots.autoMargin(gd, 'legend', {
Expand Down