-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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.
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 rangeslider titles with stacked y axes #2451
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -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'); | ||||
|
@@ -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 | ||||
|
@@ -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); | ||||
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; | ||||
|
@@ -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 | ||||
); | ||||
|
@@ -151,18 +157,31 @@ 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 | ||||
if(axisOpts.side === 'bottom') { | ||||
Titles.draw(gd, axisOpts._id + 'title', { | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice solution! |
||||
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, | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here, that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ... or is there a similar offset for range-slider-less axes? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. copied from plotly.js/src/plots/cartesian/axes.js Line 2031 in dfe13b5
That's what made me start to try and abstract this out, before deciding the logic was a little too convoluted to want to tackle it right now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great. Thanks! |
||||
'text-anchor': 'middle' | ||||
} | ||||
}); | ||||
} | ||||
|
||||
// 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 | ||||
}); | ||||
|
||||
}); | ||||
}; | ||||
|
||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -701,11 +701,6 @@ plots.linkSubplots = function(newFullData, newFullLayout, oldFullData, oldFullLa | |
|
||
var i, j, id, ax; | ||
|
||
// sort subplot lists | ||
for(var subplotType in newSubplotList) { | ||
newSubplotList[subplotType].sort(Lib.subplotSort); | ||
} | ||
|
||
for(i = 0; i < ids.length; i++) { | ||
id = ids[i]; | ||
var oldSubplot = oldSubplots[id]; | ||
|
@@ -1390,6 +1385,11 @@ plots.supplyLayoutModuleDefaults = function(layoutIn, layoutOut, fullData, trans | |
Cartesian.finalizeSubplots(layoutIn, layoutOut); | ||
} | ||
|
||
// sort subplot lists | ||
for(var subplotType in layoutOut._subplots) { | ||
layoutOut._subplots[subplotType].sort(Lib.subplotSort); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I think so, at least that was the main part of it. |
||
} | ||
|
||
// base plot module layout defaults | ||
for(i = 0; i < basePlotModules.length; i++) { | ||
_module = basePlotModules[i]; | ||
|
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"}, | ||
"yaxis": {"title": "y", "domain": [0, 0.25]}, | ||
"yaxis2": {"title": "y2", "domain": [ 0.3, 1]} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,14 +20,17 @@ | |
"xaxis": { | ||
"domain": [ 0, 0.45 ], | ||
"range": [ 1, 2 ], | ||
"rangeslider": {} | ||
"rangeslider": {}, | ||
"title": "X", | ||
"side": "top" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice touch 🥇 |
||
}, | ||
"xaxis2": { | ||
"anchor": "y2", | ||
"domain": [ 0.55, 1 ], | ||
"rangeslider": { | ||
"range": [ -2, 4 ] | ||
} | ||
}, | ||
"title": "X2" | ||
}, | ||
"yaxis": { | ||
"domain": [ 0.3, 0.8 ], | ||
|
There was a problem hiding this comment.
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