Combine geo and ternary genaral update pattern #1291
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- made useless by #946
- so that the same logic can be share for geo and ternary subplots (and maybe cartesian subplots down the road) - fix bug occurring when the first trace on a given subplot had `visible: false` (which yielded an exception)
- fix bug occurring when the first trace on a given ternary subplot had `visible: false` (which yielded an exception)
rreusser
reviewed
Jan 16, 2017
| } | ||
|
|
||
| this.traceHash = traceHash; | ||
| Plots.generalUpdatePerTraceModule(this, geoCalcData, geoLayout); |
There was a problem hiding this comment.
Lots of trace-specific code moved elsewhere. I like it already. 👏
| * @return {array} array of calcdata traces | ||
| */ | ||
| plots.getSubplotCalcData = function(calcData, type, subplotId) { | ||
| if(plots.subplotsRegistry[type] === undefined) return []; |
There was a problem hiding this comment.
if(!plots.subplotsRegistry[type]) but that's really nitpicky.
There was a problem hiding this comment.
Good call. Done in 83c2722
|
|
||
| // when a trace gets deleted, make sure that its module's | ||
| // plot method is called so that it is properly | ||
| // removed from the DOM. |
| @@ -2027,3 +2052,67 @@ plots.doCalcdata = function(gd, traces) { | |||
| calcdata[i] = cd; | |||
| } | |||
| }; | |||
|
|
|||
| plots.generalUpdatePerTraceModule = function(subplot, subplotCalcData, subplotLayout) { | |||
| var traceHashOld = subplot.traceHash, | |||
There was a problem hiding this comment.
I always feel like the mutate-existing-data approach contributes a bit to fragility, but I understand why these things are as they are.
| expect(out3.traceHash.A.length).toEqual(1); | ||
| expect(out3.traceHash.A[0][0].trace.visible).toBe(false); | ||
| expect(out3.traceHash.C.length).toEqual(1); | ||
| expect(out3.traceHash.C[0][0].trace.visible).toBe(false); |
by simple `!` checks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Cartesian, geo and ternary subplots (i.e. all SVG/d3 subplots) draw traces using the trace modules'
plotmethod which expects as input thecalcdatatraces corresponding to all traces of a given trace type on a given subplot.Previously, geo and ternary subplot used similar but not-exactly-equivalent logic to handle add/remove/modify updates. This PR - mainly by making ternary subplot use
gd.calcdatainstead ofgd._fullDatatraces - unifies the geo and ternary logic intoPlots.generalUpdatePerTraceModule(I hope you like the name). Cartesian subplots on the other hand are (still) in a world of their own - unifying all SVG/d3 subplots will have to wait.By doing so, this PR also fixes a bug where:
would lead to exception and fail to render. See commit in particular this line for more details.