-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Add ability to rename grouped traces #1919
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
Changes from 8 commits
aef38e5
d886c9e
e6f5e13
7a2316d
4e6a0aa
952e4ea
8f37fc7
dd035ef
7298a0c
64d2bcd
57dfe4a
c0089ea
42d76e0
efcef43
daac59d
6aefb41
be0e693
4943e3a
4b04131
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 |
---|---|---|
|
@@ -31,6 +31,7 @@ var anchorUtils = require('./anchor_utils'); | |
|
||
var SHOWISOLATETIP = true; | ||
var DBLCLICKDELAY = interactConstants.DBLCLICKDELAY; | ||
var BLANK_STRING_REGEX = /^[\s\r]*$/; | ||
|
||
module.exports = function draw(gd) { | ||
var fullLayout = gd._fullLayout; | ||
|
@@ -392,24 +393,54 @@ function drawTexts(g, gd) { | |
this.text(text) | ||
.call(textLayout); | ||
|
||
var origText = text; | ||
|
||
if(!this.text()) text = ' \u0020\u0020 '; | ||
|
||
var fullInput = legendItem.trace._fullInput || {}, | ||
astr; | ||
var transforms, direction; | ||
var fullInput = legendItem.trace._fullInput || {}; | ||
var needsRedraw = false; | ||
var update = {}; | ||
|
||
// N.B. this block isn't super clean, | ||
// is unfortunately untested at the moment, | ||
// and only works for for 'ohlc' and 'candlestick', | ||
// but should be generalized for other one-to-many transforms | ||
if(['ohlc', 'candlestick'].indexOf(fullInput.type) !== -1) { | ||
var transforms = legendItem.trace.transforms, | ||
direction = transforms[transforms.length - 1].direction; | ||
transforms = legendItem.trace.transforms; | ||
direction = transforms[transforms.length - 1].direction; | ||
|
||
update[direction + '.name'] = text; | ||
} else if(Registry.hasTransform(fullInput, 'groupby')) { | ||
var groupbyIndices = Registry.getTransformIndices(fullInput, 'groupby'); | ||
var index = groupbyIndices[groupbyIndices.length - 1]; | ||
|
||
var carr = Lib.keyedContainer(fullInput, 'transforms[' + index + '].groupnames', 'group', 'name'); | ||
|
||
if(BLANK_STRING_REGEX.test(origText)) { | ||
carr.remove(legendItem.trace._group); | ||
needsRedraw = true; | ||
} else { | ||
carr.set(legendItem.trace._group, text); | ||
} | ||
|
||
update = carr.constructUpdate(); | ||
} else { | ||
update.name = text; | ||
} | ||
|
||
var p = Plotly.restyle(gd, update, traceIndex); | ||
|
||
astr = direction + '.name'; | ||
// If a groupby label is deleted, it seems like we need another redraw in order | ||
// to restore the label. Otherwise it simply sets this property and the blank | ||
// string is retained. | ||
if(needsRedraw) { | ||
p = p.then(function() { | ||
return Plotly.redraw(gd); | ||
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. Hmm. We shouldn't have to do this. We should fix 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. Agreed. I'll try to make that happen. 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'm having trouble reproducing that this was ever necessary. It seems to update just fine. Giving this the 🔪 and I think it's fine. |
||
}); | ||
} | ||
else astr = 'name'; | ||
|
||
Plotly.restyle(gd, astr, text, traceIndex); | ||
return p; | ||
}); | ||
} | ||
else text.call(textLayout); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,116 @@ | ||
/** | ||
* Copyright 2012-2017, Plotly, Inc. | ||
* All rights reserved. | ||
* | ||
* This source code is licensed under the MIT license found in the | ||
* LICENSE file in the root directory of this source tree. | ||
*/ | ||
|
||
'use strict'; | ||
|
||
var nestedProperty = require('./nested_property'); | ||
|
||
// bitmask for deciding what's updated: | ||
var NONE = 0; | ||
var NAME = 1; | ||
var VALUE = 2; | ||
var BOTH = 3; | ||
|
||
module.exports = function keyedContainer(baseObj, path, keyName, valueName) { | ||
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. Interesting. I like it. @rreusser can you of other situations where this could be useful? 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. Yeah. It's also used in group styles, for one, though that case might be slightly more complicated. Also frames, though again, slightly more complicated. It's more a matter of realizing that unless we allow custom keys in the schema, abstracting this pattern seems like the only sane way forward. |
||
|
||
keyName = keyName || 'name'; | ||
valueName = valueName || 'value'; | ||
var i, arr; | ||
var changeTypes = {}; | ||
|
||
if(path && path.length) { arr = nestedProperty(baseObj, path).get(); | ||
} else { | ||
arr = baseObj; | ||
} | ||
|
||
path = path || ''; | ||
arr = arr || []; | ||
|
||
// Construct an index: | ||
var indexLookup = {}; | ||
for(i = 0; i < arr.length; i++) { | ||
indexLookup[arr[i][keyName]] = i; | ||
} | ||
|
||
var obj = { | ||
// NB: this does not actually modify the baseObj | ||
set: function(name, value) { | ||
var changeType = NONE; | ||
var idx = indexLookup[name]; | ||
if(idx === undefined) { | ||
changeType = BOTH; | ||
idx = arr.length; | ||
indexLookup[name] = idx; | ||
} else if(value !== arr[idx][valueName]) { | ||
changeType = VALUE; | ||
} | ||
var newValue = {}; | ||
newValue[keyName] = name; | ||
newValue[valueName] = value; | ||
arr[idx] = newValue; | ||
|
||
changeTypes[idx] = changeTypes[idx] | changeType; | ||
|
||
return obj; | ||
}, | ||
get: function(name) { | ||
var idx = indexLookup[name]; | ||
return idx === undefined ? undefined : arr[idx][valueName]; | ||
}, | ||
rename: function(name, newName) { | ||
var idx = indexLookup[name]; | ||
|
||
if(idx === undefined) return obj; | ||
changeTypes[idx] = changeTypes[idx] | NAME; | ||
|
||
indexLookup[newName] = idx; | ||
delete indexLookup[name]; | ||
|
||
arr[idx][keyName] = newName; | ||
|
||
return obj; | ||
}, | ||
remove: function(name) { | ||
var idx = indexLookup[name]; | ||
if(idx === undefined) return obj; | ||
for(i = idx; i < arr.length; i++) { | ||
changeTypes[i] = changeTypes[i] | BOTH; | ||
} | ||
for(i = idx; i < arr.length; i++) { | ||
indexLookup[arr[i][keyName]]--; | ||
} | ||
arr.splice(idx, 1); | ||
delete(indexLookup[name]); | ||
|
||
return obj; | ||
}, | ||
constructUpdate: function() { | ||
var astr, idx; | ||
var update = {}; | ||
var changed = Object.keys(changeTypes); | ||
for(var i = 0; i < changed.length; i++) { | ||
idx = changed[i]; | ||
astr = path + '[' + idx + ']'; | ||
if(arr[idx]) { | ||
if(changeTypes[idx] & NAME) { | ||
update[astr + '.' + keyName] = arr[idx][keyName]; | ||
} | ||
if(changeTypes[idx] & VALUE) { | ||
update[astr + '.' + valueName] = arr[idx][valueName]; | ||
} | ||
} else { | ||
update[astr] = null; | ||
} | ||
} | ||
|
||
return update; | ||
} | ||
}; | ||
|
||
return obj; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -798,7 +798,7 @@ plots.supplyDataDefaults = function(dataIn, dataOut, layout, fullLayout) { | |
|
||
for(i = 0; i < dataIn.length; i++) { | ||
trace = dataIn[i]; | ||
fullTrace = plots.supplyTraceDefaults(trace, cnt, fullLayout, i); | ||
fullTrace = plots.supplyTraceDefaults(trace, cnt, fullLayout, i, dataIn.length); | ||
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 would prefer stashing Something like 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. Sounds good. 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. Oh, actually |
||
|
||
fullTrace.index = i; | ||
fullTrace._input = trace; | ||
|
@@ -809,7 +809,12 @@ plots.supplyDataDefaults = function(dataIn, dataOut, layout, fullLayout) { | |
|
||
for(var j = 0; j < expandedTraces.length; j++) { | ||
var expandedTrace = expandedTraces[j]; | ||
var fullExpandedTrace = plots.supplyTraceDefaults(expandedTrace, cnt, fullLayout, i); | ||
var fullExpandedTrace = plots.supplyTraceDefaults(expandedTrace, cnt, fullLayout, i, dataIn.length); | ||
|
||
// The group key gets cleared. If set, pass it forward | ||
if(expandedTrace._group) { | ||
fullExpandedTrace._group = expandedTrace._group; | ||
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. Hmm. That's too bad. Do we really need this 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 maybe we could call 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. Yeah I didn't like it. I'll try to clean it up. 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. Switched to |
||
} | ||
|
||
// mutate uid here using parent uid and expanded index | ||
// to promote consistency between update calls | ||
|
@@ -940,7 +945,7 @@ plots.supplyFrameDefaults = function(frameIn) { | |
return frameOut; | ||
}; | ||
|
||
plots.supplyTraceDefaults = function(traceIn, traceOutIndex, layout, traceInIndex) { | ||
plots.supplyTraceDefaults = function(traceIn, traceOutIndex, layout, traceInIndex, inputTraceCount) { | ||
var traceOut = {}, | ||
defaultColor = Color.defaults[traceOutIndex % Color.defaults.length]; | ||
|
||
|
@@ -1013,13 +1018,13 @@ plots.supplyTraceDefaults = function(traceIn, traceOutIndex, layout, traceInInde | |
traceOut.visible = !!traceOut.visible; | ||
} | ||
|
||
plots.supplyTransformDefaults(traceIn, traceOut, layout); | ||
plots.supplyTransformDefaults(traceIn, traceOut, layout, inputTraceCount); | ||
} | ||
|
||
return traceOut; | ||
}; | ||
|
||
plots.supplyTransformDefaults = function(traceIn, traceOut, layout) { | ||
plots.supplyTransformDefaults = function(traceIn, traceOut, layout, inputTraceCount) { | ||
var globalTransforms = layout._globalTransforms || []; | ||
var transformModules = layout._transformModules || []; | ||
|
||
|
@@ -1038,7 +1043,7 @@ plots.supplyTransformDefaults = function(traceIn, traceOut, layout) { | |
if(!_module) Lib.warn('Unrecognized transform type ' + type + '.'); | ||
|
||
if(_module && _module.supplyDefaults) { | ||
transformOut = _module.supplyDefaults(transformIn, traceOut, layout, traceIn); | ||
transformOut = _module.supplyDefaults(transformIn, traceOut, layout, traceIn, inputTraceCount); | ||
transformOut.type = type; | ||
transformOut._module = _module; | ||
|
||
|
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.
hmm, there is a use case for a blank name - maybe less so for groupby than for regular traces, but sometimes you want the title for one trace to stand in for a few traces below it. You could argue then that this should just be testing for an actual empty string
''
but a space' '
should be allowed as the name.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.
okay, I could just check
=== ''
instead.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.
✅