Skip to content

Fix rangeslider titles with stacked y axes #2451

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 4 commits into from
Mar 7, 2018
Merged

Fix rangeslider titles with stacked y axes #2451

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
30af5b7
clean up range slider title positioning
alexcjohnson Mar 7, 2018
cda1db3
fix default axis anchors
alexcjohnson Mar 7, 2018
da230bc
support ax.side=top with rangesliders
alexcjohnson Mar 7, 2018
44fc08b
lint
alexcjohnson Mar 7, 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
39 changes: 28 additions & 11 deletions src/components/rangeslider/draw.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ var Plots = require('../../plots/plots');
var Lib = require('../../lib');
var Drawing = require('../drawing');
var Color = require('../color');
var Titles = require('../titles');

var Cartesian = require('../../plots/cartesian');
var Axes = require('../../plots/cartesian/axes');
Expand Down Expand Up @@ -76,8 +77,7 @@ module.exports = function(gd) {
// for all present range sliders
rangeSliders.each(function(axisOpts) {
var rangeSlider = d3.select(this),
opts = axisOpts[constants.name],
oppAxisOpts = fullLayout[Axes.id2name(axisOpts.anchor)];
opts = axisOpts[constants.name];

// update range
// Expand slider range to the axis range
Expand All @@ -96,11 +96,17 @@ module.exports = function(gd) {

// update range slider dimensions

var margin = fullLayout.margin,
graphSize = fullLayout._size,
domain = axisOpts.domain,
oppDomain = oppAxisOpts.domain,
tickHeight = (axisOpts._boundingBox || {}).height || 0;
var margin = fullLayout.margin;
var graphSize = fullLayout._size;
var domain = axisOpts.domain;
var tickHeight = (axisOpts._boundingBox || {}).height || 0;

var oppBottom = Infinity;
var subplotData = Axes.getSubplots(gd, axisOpts);
Copy link
Contributor

Choose a reason for hiding this comment

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

... I hope these patched lines won't conflict with #2364

for(var i = 0; i < subplotData.length; i++) {
var oppAxis = Axes.getFromId(gd, subplotData[i].substr(subplotData[i].indexOf('y')));
oppBottom = Math.min(oppBottom, oppAxis.domain[0]);
}

opts._id = constants.name + axisOpts._id;
opts._clipId = opts._id + '-' + fullLayout._uid;
Expand All @@ -112,7 +118,7 @@ module.exports = function(gd) {
var x = Math.round(margin.l + (graphSize.w * domain[0]));

var y = Math.round(
margin.t + graphSize.h * (1 - oppDomain[0]) +
graphSize.t + graphSize.h * (1 - oppBottom) +
tickHeight +
opts._offsetShift + constants.extraPad
);
Expand Down Expand Up @@ -151,18 +157,29 @@ module.exports = function(gd) {
// update current range
setPixelRange(rangeSlider, gd, axisOpts, opts);

// update margins
// title goes next to range slider instead of tick labels, so
// just take it over and draw it from here
Titles.draw(gd, axisOpts._id + 'title', {
propContainer: axisOpts,
propName: axisOpts._name + '.title',
placeholder: fullLayout._dfltTitle.x,
attributes: {
x: axisOpts._offset + axisOpts._length / 2,
y: y + opts._height + opts._offsetShift + 10 + 1.5 * axisOpts.titlefont.size,
'text-anchor': 'middle'
}
});
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Where to put this Titles.draw call totally confused me. First I put it after Plots.automargin below... that failed, calling Titles.draw twice: first with the adjusted margins and THEN with the un-adjusted margins - the reason being that Plots.autoMargin invokes a whole new Plotly.plot call, and in that one we get the adjusted margin, but then after that we come back here and draw the title with the original margin. Instead, I had to put it here, so that the draw with the original margin happens before we adjust the margins, then the draw with the adjusted margin happens inside Plots.autoMargin. Or I suppose I could have added some MathJax so Plots.autoMargin would be async, that would have fixed it too 🙄

@etpinard this is what got me thinking about altering our plotting pipeline with some intermediate step that's just concerned with arranging all the components but doesn't draw anything other than prerendering and stashing all the text we need sizes for.

Also, I started trying to pull out the 10 + 1.5 * axisOpts.titlefont.size that's repeated from Axes.doTicks into some sort of Titles.offset function (that's why I started changing the structure in titles/index), but gave up for now as there's too much logic concerning side and other axis attributes in doTicks.


// update margins
Plots.autoMargin(gd, opts._id, {
x: domain[0],
y: oppDomain[0],
y: oppBottom,
l: 0,
r: 0,
t: 0,
b: opts._height + margin.b + tickHeight,
pad: constants.extraPad + opts._offsetShift * 2
});

});
};

Expand Down
8 changes: 5 additions & 3 deletions src/components/titles/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,9 @@ var Color = require('../color');
var svgTextUtils = require('../../lib/svg_text_utils');
var interactConstants = require('../../constants/interactions');

var Titles = module.exports = {};
module.exports = {
draw: draw
};

var numStripRE = / [XY][0-9]* /;

Expand Down Expand Up @@ -54,7 +56,7 @@ var numStripRE = / [XY][0-9]* /;
*
* @return {selection} d3 selection of title container group
*/
Titles.draw = function(gd, titleClass, options) {
function draw(gd, titleClass, options) {
var cont = options.propContainer;
var prop = options.propName;
var placeholder = options.placeholder;
Expand Down Expand Up @@ -255,4 +257,4 @@ Titles.draw = function(gd, titleClass, options) {
el.classed('js-placeholder', isplaceholder);

return group;
};
}
37 changes: 18 additions & 19 deletions src/plots/cartesian/axes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2008,20 +2008,23 @@ axes.doTicks = function(gd, axid, skipTitle) {
// now this only applies to regular cartesian axes; colorbars and
// others ALWAYS call doTicks with skipTitle=true so they can
// configure their own titles.
var ax = axisIds.getFromId(gd, axid),
avoidSelection = d3.select(gd).selectAll('g.' + axid + 'tick'),
avoid = {
selection: avoidSelection,
side: ax.side
},
axLetter = axid.charAt(0),
gs = gd._fullLayout._size,
offsetBase = 1.5,
fontSize = ax.titlefont.size,
transform,
counterAxis,
x,
y;
var ax = axisIds.getFromId(gd, axid);

// rangeslider takes over the title so drop it here
if(ax.rangeslider && ax.rangeslider.visible && ax._boundingBox) return;

var avoidSelection = d3.select(gd).selectAll('g.' + axid + 'tick');
var avoid = {
selection: avoidSelection,
side: ax.side
};
var axLetter = axid.charAt(0);
var gs = gd._fullLayout._size;
var offsetBase = 1.5;
var fontSize = ax.titlefont.size;

var transform, counterAxis, x, y;

if(avoidSelection.size()) {
var translation = Drawing.getTranslate(avoidSelection.node().parentNode);
avoid.offsetLeft = translation.x;
Expand All @@ -2037,6 +2040,7 @@ axes.doTicks = function(gd, axid, skipTitle) {
axisIds.getFromId(gd, ax.anchor);

x = ax._offset + ax._length / 2;

if(ax.side === 'top') {
y = -titleStandoff - fontSize * (ax.showticklabels ? 1 : 0);
}
Expand All @@ -2046,11 +2050,6 @@ axes.doTicks = function(gd, axid, skipTitle) {
}
y += counterAxis._offset;

if(ax.rangeslider && ax.rangeslider.visible && ax._boundingBox) {
y += (fullLayout.height - fullLayout.margin.b - fullLayout.margin.t) *
ax.rangeslider.thickness + ax._boundingBox.height;
}

if(!avoid.side) avoid.side = 'bottom';
}
else {
Expand Down
Binary file modified test/image/baselines/candlestick_rangeslider_thai.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified test/image/baselines/range_slider.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified test/image/baselines/range_slider_axes_double.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added test/image/baselines/range_slider_axes_stacked.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified test/image/baselines/range_slider_box.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified test/image/baselines/range_slider_initial_expanded.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified test/image/baselines/range_slider_initial_valid.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified test/image/baselines/range_slider_multiple.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
11 changes: 11 additions & 0 deletions test/image/mocks/range_slider_axes_stacked.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"data": [
{"y": [1, 2, 3], "yaxis": "y2"},
{"fill": "tozeroy", "y": [30, 20, 10]}
],
"layout": {
"xaxis": {"rangeslider": {}, "title": "x", "anchor": "y"},
"yaxis": {"title": "y", "domain": [0, 0.25]},
"yaxis2": {"title": "y2", "domain": [ 0.3, 1]}
Copy link
Contributor

Choose a reason for hiding this comment

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

Referencing #2172 and more specifically #2172 (comment) - so if I understand correctly, the baseline for this mock is wrong as the desired behavior would be replicate the stacked y-axes in the range slider?

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, most likely this mock will change with #2172. Though reconciling #2172 with #2364 may be a bit of a challenge...

}
}
6 changes: 4 additions & 2 deletions test/image/mocks/range_slider_multiple.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,16 @@
"xaxis": {
"domain": [ 0, 0.45 ],
"range": [ 1, 2 ],
"rangeslider": {}
"rangeslider": {},
"title": "X"
},
"xaxis2": {
"anchor": "y2",
"domain": [ 0.55, 1 ],
"rangeslider": {
"range": [ -2, 4 ]
}
},
"title": "X2"
},
"yaxis": {
"domain": [ 0.3, 0.8 ],
Expand Down