-
-
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 14 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,41 @@ 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 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'); | ||
|
||
astr = direction + '.name'; | ||
if(BLANK_STRING_REGEX.test(origText)) { | ||
carr.remove(legendItem.trace._group); | ||
} else { | ||
carr.set(legendItem.trace._group, text); | ||
} | ||
|
||
update = carr.constructUpdate(); | ||
} else { | ||
update.name = text; | ||
} | ||
else astr = 'name'; | ||
|
||
Plotly.restyle(gd, astr, text, traceIndex); | ||
return Plotly.restyle(gd, update, traceIndex); | ||
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. fun 👍 |
||
}); | ||
} | ||
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 |
---|---|---|
|
@@ -811,6 +811,10 @@ plots.supplyDataDefaults = function(dataIn, dataOut, layout, fullLayout) { | |
var expandedTrace = expandedTraces[j]; | ||
var fullExpandedTrace = plots.supplyTraceDefaults(expandedTrace, cnt, fullLayout, i); | ||
|
||
// relink private (i.e. underscore) keys expanded trace to full expanded trace so | ||
// that transform supply-default methods can set _ keys for future use. | ||
relinkPrivateKeys(fullExpandedTrace, expandedTrace); | ||
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. Maybe add a comment here e.g.:
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. ✅ |
||
|
||
// mutate uid here using parent uid and expanded index | ||
// to promote consistency between update calls | ||
expandedTrace.uid = fullExpandedTrace.uid = fullTrace.uid + j; | ||
|
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 (`"%{group}"`), otherwise defaults to the group name', | ||||
'with trace name (`"%{group} (%{trace})"`). Available escape sequences are `%{group}`, which', | ||||
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. Shouldn't this be 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. ... but wait, 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, you're right. There are two options here:
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. Though to be honest, I'm starting to think we should intercept 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 starting to think the same. 👍 for the current implementation. 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. One small change though, right? Perhaps templateString should accept multiple objects from which to pull values and it takes the first one with a value. Then Now: Lib.templateString('%{group} (%{trace})', {group: ..., trace: ...}) Proposed: Lib.templateString('%{group} (%{name})', {group: ..., name: ...}, traceObj); So that technically 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. Interesting. The only tricky part is that to be really precise, it would have to be |
||||
'inserts the group name, and `%{trace}`, which inserts the trace name. If grouping', | ||||
'GDP data by country when more than one trace is present, for example, the', | ||||
'default "%{group} (%{trace})" 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) { | ||||
var i; | ||||
var transformOut = {}; | ||||
|
||||
function coerce(attr, dflt) { | ||||
|
@@ -83,12 +113,24 @@ exports.supplyDefaults = function(transformIn) { | |||
if(!enabled) return transformOut; | ||||
|
||||
coerce('groups'); | ||||
coerce('nameformat', layout._dataLength > 1 ? '%{group} (%{trace})' : '%{group}'); | ||||
|
||||
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,9 @@ exports.transform = function(data, state) { | |||
return newData; | ||||
}; | ||||
|
||||
|
||||
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 +195,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 +208,21 @@ 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 = Lib.templateString(opts.nameformat, { | ||||
trace: trace.name, | ||||
group: 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.
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.
✅