-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Finance refactor #2561
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
Finance refactor #2561
Changes from all commits
e979cf0
84c36a9
3870503
4c46a73
94c4560
3509b99
2849ff9
be2b523
5b6a7d5
121d171
ad1d8f0
2554482
0d80a21
82677ac
71fa112
0b7541e
f84dfae
d64fab6
dc01685
c8b03ee
302d1e6
f498bd0
c77a8a3
3f43253
13204a9
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 |
---|---|---|
|
@@ -52,7 +52,9 @@ module.exports = function style(s, gd) { | |
.each(styleBoxes) | ||
.each(stylePies) | ||
.each(styleLines) | ||
.each(stylePoints); | ||
.each(stylePoints) | ||
.each(styleCandles) | ||
.each(styleOHLC); | ||
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. 🚨 Non-blocking thinking-out-loud comment alert 🚨 We should perhaps make these trace module method (e.g. named 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. Good idea, that would be way more efficient too - instead of calling every style routine on every trace and quitting if it's not the right one, just something like: .each(function(d) { d.trace._module.legendStyle(...) }) |
||
|
||
function styleLines(d) { | ||
var trace = d[0].trace; | ||
|
@@ -207,7 +209,61 @@ module.exports = function style(s, gd) { | |
.call(Color.fill, trace.fillcolor); | ||
|
||
if(w) { | ||
p.call(Color.stroke, trace.line.color); | ||
Color.stroke(p, trace.line.color); | ||
} | ||
}); | ||
} | ||
|
||
function styleCandles(d) { | ||
var trace = d[0].trace, | ||
pts = d3.select(this).select('g.legendpoints') | ||
.selectAll('path.legendcandle') | ||
.data(trace.type === 'candlestick' && trace.visible ? [d, d] : []); | ||
pts.enter().append('path').classed('legendcandle', true) | ||
.attr('d', function(_, i) { | ||
if(i) return 'M-15,0H-8M-8,6V-6H8Z'; // increasing | ||
return 'M15,0H8M8,-6V6H-8Z'; // decreasing | ||
}) | ||
.attr('transform', 'translate(20,0)') | ||
.style('stroke-miterlimit', 1); | ||
pts.exit().remove(); | ||
pts.each(function(_, i) { | ||
var container = trace[i ? 'increasing' : 'decreasing']; | ||
var w = container.line.width, | ||
p = d3.select(this); | ||
|
||
p.style('stroke-width', w + 'px') | ||
.call(Color.fill, container.fillcolor); | ||
|
||
if(w) { | ||
Color.stroke(p, container.line.color); | ||
} | ||
}); | ||
} | ||
|
||
function styleOHLC(d) { | ||
var trace = d[0].trace, | ||
pts = d3.select(this).select('g.legendpoints') | ||
.selectAll('path.legendohlc') | ||
.data(trace.type === 'ohlc' && trace.visible ? [d, d] : []); | ||
pts.enter().append('path').classed('legendohlc', true) | ||
.attr('d', function(_, i) { | ||
if(i) return 'M-15,0H0M-8,-6V0'; // increasing | ||
return 'M15,0H0M8,6V0'; // decreasing | ||
}) | ||
.attr('transform', 'translate(20,0)') | ||
.style('stroke-miterlimit', 1); | ||
pts.exit().remove(); | ||
pts.each(function(_, i) { | ||
var container = trace[i ? 'increasing' : 'decreasing']; | ||
var w = container.line.width, | ||
p = d3.select(this); | ||
|
||
p.style('fill', 'none') | ||
.call(Drawing.dashLine, container.line.dash, w); | ||
|
||
if(w) { | ||
Color.stroke(p, container.line.color); | ||
} | ||
}); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -455,7 +455,8 @@ function drawRangePlot(rangeSlider, gd, axisOpts, opts) { | |
id: id, | ||
plotgroup: plotgroup, | ||
xaxis: xa, | ||
yaxis: ya | ||
yaxis: ya, | ||
isRangePlot: true | ||
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. I don't think this range-plot range slider fix got 🔒 anywhere. We could add a case in 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. Ah, thanks for reminding me about the rangeslider + selection fix! 🔒-> c77a8a3 |
||
}; | ||
|
||
if(isMainPlot) mainplotinfo = plotinfo; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -68,6 +68,7 @@ module.exports = { | |
'carpetlayer', | ||
'violinlayer', | ||
'boxlayer', | ||
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. Can 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. good 👀 - they probably could have before this PR, but this morning they couldn't. Now they can 0b7541e |
||
'ohlclayer', | ||
'scatterlayer' | ||
], | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -192,20 +192,24 @@ function plotOne(gd, plotinfo, cdSubplot, transitionOpts, makeOnCompleteCallback | |
// remaining plot traces should also be able to do this. Once implemented, | ||
// we won't need this - which should sometimes be a big speedup. | ||
if(plotinfo.plot) { | ||
plotinfo.plot.selectAll('g:not(.scatterlayer)').selectAll('g.trace').remove(); | ||
plotinfo.plot.selectAll('g:not(.scatterlayer):not(.ohlclayer)').selectAll('g.trace').remove(); | ||
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. This query is getting ridiculous. Can we instead loop over 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.
Somehow we need to distinguish between trace types that know how to update and those that don't - I feel like rather than adding another category for this, we should just teach the remaining types how to update. Probably actually not that hard to do, right? Just adding the right I wouldn't want to do that in this PR of course... 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. Moved to #2563 Thanks! |
||
} | ||
|
||
// plot all traces for each module at once | ||
for(var j = 0; j < modules.length; j++) { | ||
var _module = modules[j]; | ||
|
||
// skip over non-cartesian trace modules | ||
if(_module.basePlotModule.name !== 'cartesian') continue; | ||
if(!_module.plot || _module.basePlotModule.name !== 'cartesian') continue; | ||
|
||
// plot all traces of this type on this subplot at once | ||
var cdModule = getModuleCalcData(cdSubplot, _module); | ||
var cdModuleAndOthers = getModuleCalcData(cdSubplot, _module); | ||
var cdModule = cdModuleAndOthers[0]; | ||
// don't need to search the found traces again - in fact we need to NOT | ||
// so that if two modules share the same plotter we don't double-plot | ||
cdSubplot = cdModuleAndOthers[1]; | ||
|
||
if(_module.plot) _module.plot(gd, plotinfo, cdModule, transitionOpts, makeOnCompleteCallback); | ||
_module.plot(gd, plotinfo, cdModule, transitionOpts, makeOnCompleteCallback); | ||
} | ||
} | ||
|
||
|
@@ -215,6 +219,7 @@ exports.clean = function(newFullData, newFullLayout, oldFullData, oldFullLayout) | |
var oldPlots = oldFullLayout._plots || {}; | ||
|
||
var hadScatter, hasScatter; | ||
var hadOHLC, hasOHLC; | ||
var hadGl, hasGl; | ||
var i, k, subplotInfo, moduleName; | ||
|
||
|
@@ -232,28 +237,36 @@ exports.clean = function(newFullData, newFullLayout, oldFullData, oldFullLayout) | |
moduleName = oldModules[i].name; | ||
if(moduleName === 'scatter') hadScatter = true; | ||
else if(moduleName === 'scattergl') hadGl = true; | ||
else if(moduleName === 'ohlc') hadOHLC = true; | ||
} | ||
|
||
for(i = 0; i < newModules.length; i++) { | ||
moduleName = newModules[i].name; | ||
if(moduleName === 'scatter') hasScatter = true; | ||
else if(moduleName === 'scattergl') hasGl = true; | ||
else if(moduleName === 'ohlc') hasOHLC = true; | ||
} | ||
|
||
if(hadScatter && !hasScatter) { | ||
for(k in oldPlots) { | ||
subplotInfo = oldPlots[k]; | ||
if(subplotInfo.plot) { | ||
subplotInfo.plot.select('g.scatterlayer') | ||
.selectAll('g.trace') | ||
.remove(); | ||
var layersToEmpty = []; | ||
if(hadScatter && !hasScatter) layersToEmpty.push('g.scatterlayer'); | ||
if(hadOHLC && !hasOHLC) layersToEmpty.push('g.ohlclayer'); | ||
|
||
if(layersToEmpty.length) { | ||
for(var layeri = 0; layeri < layersToEmpty.length; layeri++) { | ||
for(k in oldPlots) { | ||
subplotInfo = oldPlots[k]; | ||
if(subplotInfo.plot) { | ||
subplotInfo.plot.select(layersToEmpty[layeri]) | ||
.selectAll('g.trace') | ||
.remove(); | ||
} | ||
} | ||
} | ||
|
||
oldFullLayout._infolayer.selectAll('g.rangeslider-container') | ||
.select('g.scatterlayer') | ||
.selectAll('g.trace') | ||
.remove(); | ||
oldFullLayout._infolayer.selectAll('g.rangeslider-container') | ||
.select(layersToEmpty[layeri]) | ||
.selectAll('g.trace') | ||
.remove(); | ||
} | ||
} | ||
|
||
if(hadGl && !hasGl) { | ||
|
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.
Why did you have to move this block down?
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.
Our previous users of
extraText
use onlyextraText
, no x/y/z text, so they're essentially independent of the intervening block. But finance traces useextraText
in place ofyLabel
, and still usexLabel
. Contrary to the comment (that I just moved down here withextraText
) previously if you had both parts, the x/y/z labels would obliterateextraText
.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.
Thanks for the headsup 👌