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

Fix axis line width, length, and positioning for coupled subplots #1854

Merged
merged 13 commits into from
Jul 14, 2017
Merged

Fix axis line width, length, and positioning for coupled subplots #1854

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
829cd6e
updated mock 20 to test axis line width, length, and positioning
alexcjohnson Jul 5, 2017
fe1615b
lint var statements in subroutines.lsInner
alexcjohnson Jul 10, 2017
5b495e6
plotinfo already has xaxis/yaxis stashed
alexcjohnson Jul 11, 2017
543d6b0
code cleanup in lsInner
alexcjohnson Jul 12, 2017
eb48a9d
fix axis line length logic and tweak label positioning
alexcjohnson Jul 13, 2017
aaba530
update all other baselines for the tweaked label positioning
alexcjohnson Jul 13, 2017
27d2ca2
update gl2d mocks with colorbars for tick label position tweak
alexcjohnson Jul 13, 2017
6d41a37
comment on hasSVGCartesian
alexcjohnson Jul 13, 2017
b31e60d
un-nest new lsInner helper functions
alexcjohnson Jul 13, 2017
274526a
stash _mainAxis and _anchorAxis in fullLayout axis objects
alexcjohnson Jul 13, 2017
16bae9c
make a constant for font midline alignment
alexcjohnson Jul 13, 2017
893588d
Merge branch 'master' into axis-lines
alexcjohnson Jul 13, 2017
739c998
do not lengthen disconnected axes, also test margin.pad positioning
alexcjohnson Jul 13, 2017
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
1 change: 1 addition & 0 deletions src/components/colorbar/draw.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,7 @@ module.exports = function draw(gd, id) {
ticksuffix: opts.ticksuffix,
title: opts.title,
titlefont: opts.titlefont,
showline: true,
anchor: 'free',
position: 1
},
Expand Down
281 changes: 185 additions & 96 deletions src/plot_api/subroutines.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,12 @@ function overlappingDomain(xDomain, yDomain, domains) {
}

exports.lsInner = function(gd) {
var fullLayout = gd._fullLayout,
gs = fullLayout._size,
axList = Plotly.Axes.list(gd),
i;
var fullLayout = gd._fullLayout;
var gs = fullLayout._size;
var pad = gs.p;
var axList = Plotly.Axes.list(gd);
var hasSVGCartesian = fullLayout._has('cartesian');
Copy link
Contributor

Choose a reason for hiding this comment

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

To note, this is needed as gl2d now uses cartesian (SVG layers) subplots to handle selections.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah right - I figured out that that's the reason, which is why I added SVG to the var name, but perhaps I should add the full explanation in a comment. Will become important when we start using SVG axes with GL2D plots @dfcreative

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

comment added 6d41a37

var i;

// clear axis line positions, to be set in the subplot loop below
for(i = 0; i < axList.length; i++) axList[i]._linepositions = {};
Expand Down Expand Up @@ -79,11 +81,9 @@ exports.lsInner = function(gd) {
return;
}

var xa = Plotly.Axes.getFromId(gd, subplot, 'x'),
ya = Plotly.Axes.getFromId(gd, subplot, 'y'),
xDomain = xa.domain,
yDomain = ya.domain,
plotgroupBgData = [];
var xDomain = plotinfo.xaxis.domain;
Copy link
Contributor

@etpinard etpinard Jul 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Amazing. Note that references in plotinfo in are updated linkSubplots called during supplyDefaults.

var yDomain = plotinfo.yaxis.domain;
var plotgroupBgData = [];

if(overlappingDomain(xDomain, yDomain, lowerDomains)) {
plotgroupBgData = [0];
Expand Down Expand Up @@ -124,22 +124,21 @@ exports.lsInner = function(gd) {
fullLayout._plots[subplot].bg = d3.select(this);
});

var freefinished = [];
var freeFinished = {};
subplotSelection.each(function(subplot) {
var plotinfo = fullLayout._plots[subplot];

var xa = Plotly.Axes.getFromId(gd, subplot, 'x'),
ya = Plotly.Axes.getFromId(gd, subplot, 'y');
var xa = plotinfo.xaxis;
var ya = plotinfo.yaxis;

// reset scale in case the margins have changed
xa.setScale();
ya.setScale();

if(plotinfo.bg && fullLayout._has('cartesian')) {
if(plotinfo.bg && hasSVGCartesian) {
plotinfo.bg
.call(Drawing.setRect,
xa._offset - gs.p, ya._offset - gs.p,
xa._length + 2 * gs.p, ya._length + 2 * gs.p)
xa._offset - pad, ya._offset - pad,
xa._length + 2 * pad, ya._length + 2 * pad)
.call(Color.fill, fullLayout.plot_bgcolor)
.style('stroke-width', 0);
}
Expand All @@ -165,129 +164,219 @@ exports.lsInner = function(gd) {
});


plotinfo.plot.call(Drawing.setTranslate, xa._offset, ya._offset);
plotinfo.plot.call(Drawing.setClipUrl, plotinfo.clipId);

var xlw = Drawing.crispRound(gd, xa.linewidth, 1),
ylw = Drawing.crispRound(gd, ya.linewidth, 1),
xp = gs.p + ylw,
xpathPrefix = 'M' + (-xp) + ',',
xpathSuffix = 'h' + (xa._length + 2 * xp),
showfreex = xa.anchor === 'free' &&
freefinished.indexOf(xa._id) === -1,
freeposx = gs.h * (1 - (xa.position||0)) + ((xlw / 2) % 1),
showbottom =
(xa.anchor === ya._id && (xa.mirror || xa.side !== 'top')) ||
xa.mirror === 'all' || xa.mirror === 'allticks' ||
(xa.mirrors && xa.mirrors[ya._id + 'bottom']),
bottompos = ya._length + gs.p + xlw / 2,
showtop =
(xa.anchor === ya._id && (xa.mirror || xa.side === 'top')) ||
xa.mirror === 'all' || xa.mirror === 'allticks' ||
(xa.mirrors && xa.mirrors[ya._id + 'top']),
toppos = -gs.p - xlw / 2,

// shorten y axis lines so they don't overlap x axis lines
yp = gs.p,
// except where there's no x line
// TODO: this gets more complicated with multiple x and y axes
ypbottom = showbottom ? 0 : xlw,
yptop = showtop ? 0 : xlw,
ypathSuffix = ',' + (-yp - yptop) +
'v' + (ya._length + 2 * yp + yptop + ypbottom),
showfreey = ya.anchor === 'free' &&
freefinished.indexOf(ya._id) === -1,
freeposy = gs.w * (ya.position||0) + ((ylw / 2) % 1),
showleft =
(ya.anchor === xa._id && (ya.mirror || ya.side !== 'right')) ||
ya.mirror === 'all' || ya.mirror === 'allticks' ||
(ya.mirrors && ya.mirrors[xa._id + 'left']),
leftpos = -gs.p - ylw / 2,
showright =
(ya.anchor === xa._id && (ya.mirror || ya.side === 'right')) ||
ya.mirror === 'all' || ya.mirror === 'allticks' ||
(ya.mirrors && ya.mirrors[xa._id + 'right']),
rightpos = xa._length + gs.p + ylw / 2;
plotinfo.plot
.call(Drawing.setTranslate, xa._offset, ya._offset)
.call(Drawing.setClipUrl, plotinfo.clipId);

function shouldShowLine(ax, counterAx, side) {
return (ax.anchor === counterAx._id && (ax.mirror || ax.side === side)) ||
ax.mirror === 'all' || ax.mirror === 'allticks' ||
(ax.mirrors && ax.mirrors[counterAx._id + side]);
}

var xIsFree = xa.anchor === 'free';
var showFreeX = xIsFree && !freeFinished[xa._id];
var showBottom = shouldShowLine(xa, ya, 'bottom');
var showTop = shouldShowLine(xa, ya, 'top');

var yIsFree = ya.anchor === 'free';
var showFreeY = yIsFree && !freeFinished[ya._id];
var showLeft = shouldShowLine(ya, xa, 'left');
var showRight = shouldShowLine(ya, xa, 'right');

var xlw = Drawing.crispRound(gd, xa.linewidth, 1);
var ylw = Drawing.crispRound(gd, ya.linewidth, 1);

function findMainAxis(ax) {
return ax.overlaying ? Plotly.Axes.getFromId(gd, ax.overlaying) : ax;
Copy link
Contributor

Choose a reason for hiding this comment

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

Non- ⛔ : might be nice to stash references to overlaying and anchor axes in plotinfo during makeSubplotData and 🔪 those Axes.getFromId

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good idea. Actually makeSubplotData didn't really seem the place for this, it duplicated a lot of effort that should have just been done once per axis. I put this all into plots.linkSubplots. 274526a

}

function findCounterAxes(ax) {
Copy link
Contributor

@etpinard etpinard Jul 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Would it be too much to ask to move those new find* functions outside of the subplotSelection.each scope? I don't think compilers can optimize that still meaning these functions are redefined for every subplot.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ah, I actually meant to un-nest these, thanks for reminding me. b31e60d

var counterAxes = [];
var anchorAx = Plotly.Axes.getFromId(gd, ax.anchor);
if(anchorAx) {
var counterMain = findMainAxis(anchorAx);
if(counterAxes.indexOf(counterMain) === -1) {
counterAxes.push(counterMain);
}
for(var i = 0; i < axList.length; i++) {
if(axList[i].overlaying === counterMain._id &&
counterAxes.indexOf(axList[i]) === -1
) {
counterAxes.push(axList[i]);
}
}
}
return counterAxes;
}

function findLineWidth(axes, side) {
for(var i = 0; i < axes.length; i++) {
var ax = axes[i];
if(ax.anchor !== 'free' && shouldShowLine(ax, {_id: ax.anchor}, side)) {
return Drawing.crispRound(gd, ax.linewidth);
}
}
}

function findCounterAxisLineWidth(ax, subplotCounterLineWidth,
subplotCounterIsShown, side) {
if(subplotCounterIsShown) return subplotCounterLineWidth;

var i;

// find all counteraxes for this one, then of these, find the
// first one that has a visible line on this side
var mainAxis = findMainAxis(ax);
var counterAxes = findCounterAxes(mainAxis);

var lineWidth = findLineWidth(counterAxes, side);
if(lineWidth) return lineWidth;

for(i = 0; i < axList.length; i++) {
if(axList[i].overlaying === mainAxis._id) {
counterAxes = findCounterAxes(axList[i]);
lineWidth = findLineWidth(counterAxes, side);
if(lineWidth) return lineWidth;
}
}
return 0;
}

/*
* x lines get longer where they meet y lines, to make a crisp corner
* free x lines are not excluded - they don't necessarily *meet* the
* y lines, but it still looks good if the x line reaches to the ends
* of the y lines, especially in the case of a free axis parallel to
* an anchored axis, like this:
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree 100%. Thanks for documenting.

*
* |
* |
* +-----
* x1
* ------
* ^ x2
*/
var xLinesXLeft = -pad - findCounterAxisLineWidth(xa, ylw, showLeft, 'left');
var xLinesXRight = xa._length + pad + findCounterAxisLineWidth(xa, ylw, showRight, 'right');
var xLinesYFree = gs.h * (1 - (xa.position || 0)) + ((xlw / 2) % 1);
var xLinesYBottom = ya._length + pad + xlw / 2;
var xLinesYTop = -pad - xlw / 2;

/*
* y lines do not get longer when they meet x axes, because the
* x axis already filled that space and we don't want to double-fill.
* BUT they get longer if they're free axes, for the same reason as
* we do not exclude x axes:
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

Choose a reason for hiding this comment

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

mock 20 looks amazing.

*
* | |
* y2| y1|
* | |
* >| +-----
*
* arguably if the free y axis is over top of the anchored x axis,
* we don't want to do this... but that's a weird edge case, doesn't
* seem worth adding a lot of complexity for.
*/
var yLinesYBottom = ya._length + pad + (yIsFree ?
findCounterAxisLineWidth(ya, xlw, showBottom, 'bottom') :
0);
var yLinesYTop = -pad - (yIsFree ?
findCounterAxisLineWidth(ya, xlw, showTop, 'top') :
0);
var yLinesXFree = gs.w * (ya.position || 0) + ((ylw / 2) % 1);
var yLinesXLeft = -pad - ylw / 2;
var yLinesXRight = xa._length + pad + ylw / 2;

function xLinePath(y, showThis) {
if(!showThis) return '';
return 'M' + xLinesXLeft + ',' + y + 'H' + xLinesXRight;
}

function yLinePath(x, showThis) {
if(!showThis) return '';
return 'M' + x + ',' + yLinesYTop + 'V' + yLinesYBottom;
}

// save axis line positions for ticks, draggers, etc to reference
// each subplot gets an entry:
// [left or bottom, right or top, free, main]
// main is the position at which to draw labels and draggers, if any
xa._linepositions[subplot] = [
showbottom ? bottompos : undefined,
showtop ? toppos : undefined,
showfreex ? freeposx : undefined
showBottom ? xLinesYBottom : undefined,
showTop ? xLinesYTop : undefined,
showFreeX ? xLinesYFree : undefined
];
if(xa.anchor === ya._id) {
xa._linepositions[subplot][3] = xa.side === 'top' ?
toppos : bottompos;
xLinesYTop : xLinesYBottom;
}
else if(showfreex) {
xa._linepositions[subplot][3] = freeposx;
else if(showFreeX) {
xa._linepositions[subplot][3] = xLinesYFree;
}

ya._linepositions[subplot] = [
showleft ? leftpos : undefined,
showright ? rightpos : undefined,
showfreey ? freeposy : undefined
showLeft ? yLinesXLeft : undefined,
showRight ? yLinesXRight : undefined,
showFreeY ? yLinesXFree : undefined
];
if(ya.anchor === xa._id) {
ya._linepositions[subplot][3] = ya.side === 'right' ?
rightpos : leftpos;
yLinesXRight : yLinesXLeft;
}
else if(showfreey) {
ya._linepositions[subplot][3] = freeposy;
else if(showFreeY) {
ya._linepositions[subplot][3] = yLinesXFree;
}

// translate all the extra stuff to have the
// same origin as the plot area or axes
var origin = 'translate(' + xa._offset + ',' + ya._offset + ')',
originx = origin,
originy = origin;
if(showfreex) {
originx = 'translate(' + xa._offset + ',' + gs.t + ')';
toppos += ya._offset - gs.t;
bottompos += ya._offset - gs.t;
var origin = 'translate(' + xa._offset + ',' + ya._offset + ')';
var originX = origin;
var originY = origin;
if(showFreeX) {
originX = 'translate(' + xa._offset + ',' + gs.t + ')';
xLinesYTop += ya._offset - gs.t;
xLinesYBottom += ya._offset - gs.t;
}
if(showfreey) {
originy = 'translate(' + gs.l + ',' + ya._offset + ')';
leftpos += xa._offset - gs.l;
rightpos += xa._offset - gs.l;
if(showFreeY) {
originY = 'translate(' + gs.l + ',' + ya._offset + ')';
yLinesXLeft += xa._offset - gs.l;
yLinesXRight += xa._offset - gs.l;
}

if(fullLayout._has('cartesian')) {
if(hasSVGCartesian) {
plotinfo.xlines
.attr('transform', originx)
.attr('transform', originX)
.attr('d', (
(showbottom ? (xpathPrefix + bottompos + xpathSuffix) : '') +
(showtop ? (xpathPrefix + toppos + xpathSuffix) : '') +
(showfreex ? (xpathPrefix + freeposx + xpathSuffix) : '')) ||
xLinePath(xLinesYBottom, showBottom) +
xLinePath(xLinesYTop, showTop) +
xLinePath(xLinesYFree, showFreeX)) ||
// so it doesn't barf with no lines shown
'M0,0')
.style('stroke-width', xlw + 'px')
.call(Color.stroke, xa.showline ?
xa.linecolor : 'rgba(0,0,0,0)');
plotinfo.ylines
.attr('transform', originy)
.attr('transform', originY)
.attr('d', (
(showleft ? ('M' + leftpos + ypathSuffix) : '') +
(showright ? ('M' + rightpos + ypathSuffix) : '') +
(showfreey ? ('M' + freeposy + ypathSuffix) : '')) ||
yLinePath(yLinesXLeft, showLeft) +
yLinePath(yLinesXRight, showRight) +
yLinePath(yLinesXFree, showFreeY)) ||
'M0,0')
.attr('stroke-width', ylw + 'px')
.style('stroke-width', ylw + 'px')
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch. Is .attr('stroke-width', /* */) a thing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, a lot of properties in SVG you can use either .attr or .style, with slightly different rules but also very different precedence - see https://css-tricks.com/presentation-attributes-vs-inline-styles/ - attributes are about the lowest, while inline styles are nearly the highest.
@monfera and I had a discussion about this a few days ago, I think for our purposes .style should be preferred.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ha, interesting. It's something we might to standardize in strict-d3.js.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think stroke-width as an attribute doesn't need the 'px' but also, the question kept brewing in my mind and I like the (user-experienced) flexibility of using attributes where possible, as it's easy to restyle them. With D3 the typical usage is inline styling of the element, which is for our current context, the highest priority, ie. the user would have a hard time overriding it (there are two main ways: 1) user reselects with D3 and changes the inline style; 2) user specifies !important styling in the style sheet. So, using attr would be more convenient for users if they choose to restyle via CSS.

Copy link
Contributor

Choose a reason for hiding this comment

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

... also, there's the thing that Microsoft browsers (IE, Edge) don't have full CSS support, so if the decision is to go with style as much as we can, then we still must use attr for transform and possibly some more presentation attributes, ie. some style->attr switch may need to happen during MS testing

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right, transform should stay as an attr for MS support. I think even some things like x and y that we've always provided as attributes can be styles. I think my own proclivity is to treat positioning as attributes and everything else as style, but I'm sure I've broken that rule. Might be worthwhile to create a list of property names we use and whether each should be used as a style or attribute? And then yes, we could enforce that in strict-d3.

I'm sympathetic to the idea of giving users more control, but in the end I think it would be a mistake to encourage overriding internals of the plot with CSS - we should push people to use the plot JSON instead - for three reasons:

  1. Portability: with outside CSS the figure JSON no longer fully describes the plot. Even if you don't care about sharing a plot to another server, you might care about the image server or - depending on how the user applied their styling - even in-browser downloading could be affected.
  2. Accidental overrides: we use a lot of generic class names like .point, .xtitle, that could get mangled easily if the page does something else with those classes.
  3. Forward compatibility: obviously anyone can peek inside our elements and find selectors for whatever elements they'd like to style, but we've never published this as part of the API, and I don't want to give up the right to refactor our structures and rename classes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes these points are all true. We don't currently go out of our way to dissuade scenegraph class based styling - we don't generate mangled or one-off class names or otherwise actively dissuade external styling. Also on the forum, SO and gh questions sometimes our answer is that a particular wish isn't supported but can be done with a d3.selectAll('whatever').style(...) and our scenegraph class is human friendly so I over-inferred 😄 That and the upside of not having to have heuristics for when to switch to attr. Btw. as a developer either way is fine as long as there's a guideline like what you mentioned above @alexcjohnson

Not sure if the emerging linter solution already covers it, I think this is the list of attributes (ie. all presentation attributes) that can go either way.

Unfortunately it's quite Venn-diagrammey and common stuff like x, dx, r etc. ie. drivers of the most basic geometry are not on it so even in the future, those things won't be able to use CSS transitions/animations. (Maybe interesting is that the SMIL deprecation note got retracted.)

.call(Color.stroke, ya.showline ?
ya.linecolor : 'rgba(0,0,0,0)');
}

plotinfo.xaxislayer.attr('transform', originx);
plotinfo.yaxislayer.attr('transform', originy);
plotinfo.xaxislayer.attr('transform', originX);
plotinfo.yaxislayer.attr('transform', originY);
plotinfo.gridlayer.attr('transform', origin);
plotinfo.zerolinelayer.attr('transform', origin);
plotinfo.draglayer.attr('transform', origin);

// mark free axes as displayed, so we don't draw them again
if(showfreex) { freefinished.push(xa._id); }
if(showfreey) { freefinished.push(ya._id); }
if(showFreeX) freeFinished[xa._id] = 1;
if(showFreeY) freeFinished[ya._id] = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Smart. I usually set keys like this as true. But 1 is truthy and three characters shorter!

});

Plotly.Axes.makeClipPaths(gd);
Expand Down
Loading