-
-
Notifications
You must be signed in to change notification settings - Fork 2k
Multi-axis Shapes #7666
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
base: master
Are you sure you want to change the base?
Multi-axis Shapes #7666
Conversation
src/components/shapes/helpers.js
Outdated
| return extractedCoordinates; | ||
| }; | ||
|
|
||
| exports.countDefiningCoords = function(path, isNotPath) { |
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.
a more intuitive function signature here would be function(shapeType, path) I think
| // for each path command, check if there is a drawn coordinate | ||
| var segmentType = segment.charAt(0); | ||
| var hasDrawnX = constants.paramIsX[segmentType].drawn !== undefined; | ||
| var hasDrawnY = constants.paramIsY[segmentType].drawn !== undefined; | ||
| if(hasDrawnX || hasDrawnY) coordCount++; |
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. H and V segments cause an odd usability issue I hadn't considered: if a path contains H and V segments then the expected lengths of xref and yref may be different, and the corresponding list items will be "out of sync" with respect to which path segments they correspond to (i.e. it will not always be the case that xref[n] and yref[n] refer to the same path segment).
For example here is a rectangular path: M0,0H10V5H0Z (see visualization)
There are four segments (not counting Z), but only 3 x-values and 2 y-values are needed to define the shape. So you would have:
{
"path": "M0,0H10V5H0Z",
"xref": ["x", "x2", "x"],
"yref": ["y", "y2"],
}which is a bit odd, and sort a high cognitive load for the user to figure out the right xref and yref lists.
That said, it seems probably better than the other alternative that comes to mind, which would be to force the user to add an xref and a yref for every single segment, even if it will be ignored for some segments.
src/plots/cartesian/axes.js
Outdated
| * extraOption: aside from existing axes with this letter, what non-axis value is allowed? | ||
| * Only required if it's different from `dflt` | ||
| */ | ||
| axes.coerceRefArray = function(containerIn, containerOut, gd, attr, expectedLen) { |
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.
dflt and extraOption should probably be arguments to this function
| var axRef = containerIn[refAttr]; | ||
| var dflt = axlist.length ? axlist[0] : 'paper'; | ||
|
|
||
| // Handle array length mismatch |
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.
Should probably raise a warning in both of these cases
src/plots/cartesian/axes.js
Outdated
|
|
||
| // Check all references, replace with default if invalid | ||
| for(var i = 0; i < axRef.length; i++) { | ||
| if(!(axRef[i] === 'paper' || cartesianConstants.idRegex[axLetter].test(axRef[i]))) { |
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, unless I'm missing something I think you want to use the logic from axes.coerceRef here: var axlist = gd._fullLayout._subplots[axLetter + 'axis'];
Because you don't just want to check if it's theoretically a valid axis reference; you want to check if it corresponds to an axis that actually exists in the plot.
| } | ||
| } | ||
|
|
||
| containerOut[refAttr] = axRef; |
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.
The core Lib.coerce() function uses more complex logic here for setting the attribute in the out container -- I'm not 100% sure whether that's needed here, but something to verify.
src/plots/cartesian/axes.js
Outdated
| */ | ||
| axes.getRefType = function(ar) { | ||
| if(ar === undefined) { return ar; } | ||
| if(Array.isArray(ar)) { return 'array'; } |
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.
I don't think this is needed, since anywhere in the codebase where getRefType() would be called on an array probably needs a whole separate code path to handle ref arrays.
Description:
Extends
xrefandyrefto accept arrays, allowing shapes to span multiple subplots with each vertex anchored to a different axis. See #7151 for more information.Example:
Progress:
xrefandyrefto allow array values