Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 2k
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
Uh oh!
There was an error while loading. Please reload this page.
Finance refactor #2561
Changes from all commits
e979cf084c36a938705034c46a7394c45603509b992849ff9be2b5235b6a7d5121d171ad1d8f025544820d80a2182677ac71fa1120b7541ef84dfaed64fab6dc01685c8b03ee302d1e6f498bd0c77a8a33f4325313204a9File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
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); | ||
Contributor 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 CollaboratorAuthor 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 | ||
Contributor 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 CollaboratorAuthor 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', | ||
Contributor 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 CollaboratorAuthor 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(); | ||
Contributor 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 CollaboratorAuthor 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... Contributor 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) { | ||
Uh oh!
There was an error while loading. Please reload this page.
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
extraTextuse onlyextraText, no x/y/z text, so they're essentially independent of the intervening block. But finance traces useextraTextin 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 👌