-
-
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 7 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,57 @@ 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 i, 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(fullInput.transforms) { | ||
for(i = fullInput.transforms.length - 1; i >= 0; i--) { | ||
if(fullInput.transforms[i].type === 'groupby') { | ||
break; | ||
} | ||
} | ||
|
||
var carr = Lib.keyedContainer(fullInput, 'transforms[' + i + '].groupnames', 'group', 'name'); | ||
|
||
if(BLANK_STRING_REGEX.test(origText)) { | ||
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, 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 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. okay, I could just check 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. ✅ |
||
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; | ||
|
||
|
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -35,6 +35,35 @@ exports.attributes = { | |||
'with `x` [1, 3] and one trace with `x` [2, 4].' | ||||
].join(' ') | ||||
}, | ||||
nameformat: { | ||||
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. Bringing in @alexcjohnson @cldougl @chriddyp @cpsievert @monfera what do you think of this syntax? A lot of existing attributes could benefit form special formatting character like this one. For example, trace = {
y: [1, 2, 1]
marker: {
size: [20, 30, 10]
},
hovertext: '%marker.size'
} would be a nice shortcut to show each point's 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. Personally, I 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. ... and I would make user spell out 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 really like the 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, this is cool! I hadn't thought about extending this to other keys but I can imagine that going off in all sorts of directions in the future (
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, I see. Like a general substitution framework? Yeah, the goal here was just a one-off custom substitution in which it only knows about a couple properties. But definitely possibilities. The only thing then would be choosing awkward names 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. @etpinard @alexcjohnson I've added 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. See: Line 748 in 57dfe4a
|
||||
valType: 'string', | ||||
description: [ | ||||
'Pattern by which grouped traces are named. If only one trace is present,', | ||||
'defaults to the group name (`"%g"`), otherwise defaults to the group name', | ||||
'with trace name (`"%g (%t)"`). Available escape sequences are `%g`, which', | ||||
'inserts the group name, and `%t`, which inserts the trace name. If grouping', | ||||
'GDP data by country when more than one trace is present, for example, the', | ||||
'default "%g (%t)" would return "Monaco (GDP per capita)".' | ||||
].join(' ') | ||||
}, | ||||
groupnames: { | ||||
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. I missed this during my first review. I'm not a fan of We could instead declare Maybe we could Actually, do we even need a I apologize for not catching this sooner. 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.
Not quite - the name will be set by
Or ambiguity, if the names were not perfectly duplicated. What would that mean?
That sounds great! so each entry 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. The main problem is 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.
Exactly. I'd vote for 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. I think that's the best option. 👍 Then it really is just a style property (which could be set anyway and in fact we'd be doing something surprising not to use what's already possible). 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. FWIW, IMO, TBH, I'm even happier now that I took the time to implement keyedContainer… 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 call 👍 |
||||
_isLinkedToArray: 'groupname', | ||||
group: { | ||||
valType: 'string', | ||||
role: 'info', | ||||
description: [ | ||||
'An group to which this name applies. If a `group` and `name` are specified,', | ||||
'that name overrides the `nameformat` for that group\'s trace.' | ||||
].join(' ') | ||||
}, | ||||
name: { | ||||
valType: 'string', | ||||
role: 'info', | ||||
description: [ | ||||
'Trace names assigned to the grouped traces of the corresponding `group`.' | ||||
].join(' ') | ||||
} | ||||
}, | ||||
styles: { | ||||
_isLinkedToArray: 'style', | ||||
target: { | ||||
|
@@ -71,7 +100,8 @@ exports.attributes = { | |||
* @return {object} transformOut | ||||
* copy of transformIn that contains attribute defaults | ||||
*/ | ||||
exports.supplyDefaults = function(transformIn) { | ||||
exports.supplyDefaults = function(transformIn, traceOut, layout, traceIn, inputTraceCount) { | ||||
var i; | ||||
var transformOut = {}; | ||||
|
||||
function coerce(attr, dflt) { | ||||
|
@@ -83,12 +113,24 @@ exports.supplyDefaults = function(transformIn) { | |||
if(!enabled) return transformOut; | ||||
|
||||
coerce('groups'); | ||||
coerce('nameformat', inputTraceCount > 1 ? '%g (%t)' : '%g'); | ||||
|
||||
var nameFormatIn = transformIn.groupnames; | ||||
var nameFormatOut = transformOut.groupnames = []; | ||||
|
||||
if(nameFormatIn) { | ||||
for(i = 0; i < nameFormatIn.length; i++) { | ||||
nameFormatOut[i] = {}; | ||||
Lib.coerce(nameFormatIn[i], nameFormatOut[i], exports.attributes.groupnames, 'group'); | ||||
Lib.coerce(nameFormatIn[i], nameFormatOut[i], exports.attributes.groupnames, 'name'); | ||||
} | ||||
} | ||||
|
||||
var styleIn = transformIn.styles; | ||||
var styleOut = transformOut.styles = []; | ||||
|
||||
if(styleIn) { | ||||
for(var i = 0; i < styleIn.length; i++) { | ||||
for(i = 0; i < styleIn.length; i++) { | ||||
styleOut[i] = {}; | ||||
Lib.coerce(styleIn[i], styleOut[i], exports.attributes.styles, 'target'); | ||||
Lib.coerce(styleIn[i], styleOut[i], exports.attributes.styles, 'value'); | ||||
|
@@ -130,9 +172,15 @@ exports.transform = function(data, state) { | |||
return newData; | ||||
}; | ||||
|
||||
function computeName(pattern, traceName, groupName) { | ||||
return pattern.replace(/%g/g, groupName) | ||||
.replace(/%t/g, traceName); | ||||
} | ||||
|
||||
|
||||
function transformOne(trace, state) { | ||||
var i, j, k, attr, srcArray, groupName, newTrace, transforms, arrayLookup; | ||||
var groupNameObj; | ||||
|
||||
var opts = state.transform; | ||||
var groups = trace.transforms[state.transformIndex].groups; | ||||
|
@@ -153,6 +201,10 @@ function transformOne(trace, state) { | |||
styleLookup[styles[i].target] = styles[i].value; | ||||
} | ||||
|
||||
if(opts.groupnames) { | ||||
groupNameObj = Lib.keyedContainer(opts, 'groupnames', 'group', 'name'); | ||||
} | ||||
|
||||
// An index to map group name --> expanded trace index | ||||
var indexLookup = {}; | ||||
|
||||
|
@@ -162,7 +214,18 @@ function transformOne(trace, state) { | |||
|
||||
// Start with a deep extend that just copies array references. | ||||
newTrace = newData[i] = Lib.extendDeepNoArrays({}, trace); | ||||
newTrace.name = groupName; | ||||
newTrace._group = groupName; | ||||
|
||||
var suppliedName = null; | ||||
if(groupNameObj) { | ||||
suppliedName = groupNameObj.get(groupName); | ||||
} | ||||
|
||||
if(suppliedName) { | ||||
newTrace.name = suppliedName; | ||||
} else { | ||||
newTrace.name = computeName(opts.nameformat, trace.name, groupName); | ||||
} | ||||
|
||||
// In order for groups to apply correctly to other transform data (e.g. | ||||
// a filter transform), we have to break the connection and clone the | ||||
|
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.
Might be nice to add a lib function similar to
Registry.traceIs
for transforms e.g.Registry,hasTransform('groupby')
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.
Added
Regsitry.hasTransform
andRegistry.getTransformIndices
to accomplish this. It's less efficient than the above code, but so marginally that I think it's fine. ✅