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 all 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
The table of contents is too big for display.
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
10 changes: 9 additions & 1 deletion src/constants/alignment.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,5 +30,13 @@ module.exports = {
top: 0
},
// multiple of fontSize to get the vertical offset between lines
LINE_SPACING: 1.3
LINE_SPACING: 1.3,

// multiple of fontSize to shift from the baseline to the midline
// (to use when we don't calculate this shift from Drawing.bBox)
// To be precise this should be half the cap height (capital letter)
// of the font, and according to wikipedia:
// an "average" font might have a cap height of 70% of the em
// https://en.wikipedia.org/wiki/Em_(typography)#History
MID_SHIFT: 0.35
};
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 @@ -42,10 +42,16 @@ 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);

// _has('cartesian') means SVG specifically, not GL2D - but GL2D
// can still get here because it makes some of the SVG structure
// for shared features like selections.
var hasSVGCartesian = fullLayout._has('cartesian');
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 @@ -80,11 +86,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;
var yDomain = plotinfo.yaxis.domain;
var plotgroupBgData = [];

if(overlappingDomain(xDomain, yDomain, lowerDomains)) {
plotgroupBgData = [0];
Expand Down Expand Up @@ -125,22 +129,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,7 +168,7 @@ exports.lsInner = function(gd) {
'height': ya._length
});

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

var plotClipId;
var layerClipId;
Expand All @@ -191,126 +194,153 @@ exports.lsInner = function(gd) {
// to DRY up Drawing.setClipUrl calls downstream
plotinfo.layerClipId = layerClipId;

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;
var xIsFree = !xa._anchorAxis;
var showFreeX = xIsFree && !freeFinished[xa._id];
var showBottom = shouldShowLine(xa, ya, 'bottom');
var showTop = shouldShowLine(xa, ya, 'top');

var yIsFree = !ya._anchorAxis;
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);

/*
* x lines get longer where they meet y lines, to make a crisp corner.
* The x lines get the padding (margin.pad) plus the y line width to
* fill up the corner nicely. Free x lines are excluded - they always
* span exactly the data area of the plot
*
* | XXXXX
* | XXXXX
* |
* +------
* x1
* -----
* x2
*/
var leftYLineWidth = findCounterAxisLineWidth(gd, xa, ylw, showLeft, 'left', axList);
var xLinesXLeft = (!xIsFree && leftYLineWidth) ?
(-pad - leftYLineWidth) : 0;
var rightYLineWidth = findCounterAxisLineWidth(gd, xa, ylw, showRight, 'right', axList);
var xLinesXRight = xa._length + ((!xIsFree && rightYLineWidth) ?
(pad + rightYLineWidth) : 0);
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 that meet x axes get longer only by margin.pad, because
* the x axes fill in the corner space. Free y axes, like free x axes,
* always span exactly the data area of the plot
*
* | | XXXX
* y2| y1| XXXX
* | | XXXX
* |
* +-----
*/
var connectYBottom = !yIsFree && findCounterAxisLineWidth(
gd, ya, xlw, showBottom, 'bottom', axList);
var yLinesYBottom = ya._length + (connectYBottom ? pad : 0);
var connectYTop = !yIsFree && findCounterAxisLineWidth(
gd, ya, xlw, showTop, 'top', axList);
var yLinesYTop = connectYTop ? -pad : 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) {
if(xa._anchorAxis === ya) {
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) {
if(ya._anchorAxis === xa) {
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 All @@ -320,6 +350,65 @@ exports.lsInner = function(gd) {
return gd._promises.length && Promise.all(gd._promises);
};

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

function findCounterAxes(gd, ax, axList) {
var counterAxes = [];
var anchorAx = ax._anchorAxis;
if(anchorAx) {
var counterMain = anchorAx._mainAxis;
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(gd, axes, side) {
for(var i = 0; i < axes.length; i++) {
var ax = axes[i];
var anchorAx = ax._anchorAxis;
if(anchorAx && shouldShowLine(ax, anchorAx, side)) {
return Drawing.crispRound(gd, ax.linewidth);
}
}
}

function findCounterAxisLineWidth(gd, ax, subplotCounterLineWidth,
subplotCounterIsShown, side, axList) {
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 = ax._mainAxis;
var counterAxes = findCounterAxes(gd, mainAxis, axList);

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

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

exports.drawMainTitle = function(gd) {
var fullLayout = gd._fullLayout;

Expand Down
Loading