Skip to content
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

Mult axis formatting #6334

Merged
merged 63 commits into from Dec 22, 2022
Merged
Show file tree
Hide file tree
Changes from 56 commits
Commits
Show all changes
63 commits
Select commit Hold shift + click to select a range
e4a81e3
Add shift parameter and set defaults accordingly
hannahker Sep 29, 2022
de79261
Set defaults
hannahker Sep 29, 2022
18f21cb
Simplify logic
hannahker Oct 3, 2022
0e85776
Fix coercion logic
hannahker Oct 3, 2022
ba0c587
Include shift logic during drawing
hannahker Oct 4, 2022
2731eb8
Add baseline image and fix syntax for tests
hannahker Oct 4, 2022
c535c56
Fixes for tests
hannahker Oct 4, 2022
a8fcadc
Update plot schema
hannahker Oct 5, 2022
0f93feb
Fix syntax
hannahker Oct 5, 2022
d79ef96
pass axShifts in opts & bypass undefined axShifts
archmoj Oct 5, 2022
b1d10c5
Shift axis line if applicable
hannahker Oct 5, 2022
3fa0f0c
Add new mocks and set default position according to overlaying domain
hannahker Oct 6, 2022
50fe1fd
Shift axes according to overlaying group
hannahker Oct 6, 2022
1984b7e
Clean up code to increment shifts
hannahker Oct 6, 2022
b9c77b8
Set shift valType to any
hannahker Oct 13, 2022
8a420d6
Add jasmine tests for defaults and update baselines
hannahker Oct 13, 2022
8d0c9eb
Fix syntax
hannahker Oct 13, 2022
5528a35
Enable numeric shift values
hannahker Oct 14, 2022
fbdc5a4
Fix zerolines bug
hannahker Oct 14, 2022
d742cc5
Fix logic in shift values
hannahker Oct 14, 2022
c2ba6b0
Push axis to layout in jasmine test and add baseline
hannahker Oct 14, 2022
cb66b12
Fix syntax
hannahker Oct 14, 2022
38f17b2
Add more jasmine tests for defaults relative to shift
hannahker Oct 14, 2022
bbf682f
Fix bugs with scroll and drag interactions
hannahker Oct 21, 2022
0175979
Simplify syntax
hannahker Oct 21, 2022
7a1c9d8
Adjust mock to have variable axis sizes
hannahker Oct 24, 2022
5aee58b
Set axis shift after drawing
hannahker Oct 27, 2022
fd098a9
Push out axes by estimated depth
hannahker Nov 2, 2022
79c2c74
Move axis line drawing to axes file
hannahker Nov 4, 2022
bc9bfa2
Avoid drawing lines when not necessary
hannahker Nov 4, 2022
5ffbdd2
Calculate depth without axis titles
hannahker Nov 8, 2022
168b3db
Remove arrow functions
hannahker Nov 10, 2022
472efb9
Fix failing dragbox test and mock validation
hannahker Nov 10, 2022
0c33429
Add hard coded padding and titles to axes
hannahker Nov 10, 2022
83e7f82
Remove whitespace
hannahker Nov 10, 2022
6e71f53
Replace mocks
hannahker Nov 10, 2022
28eadb0
Merge pull request #6357 from plotly/mult-axis-formatting-depth
hannahker Nov 17, 2022
6d5ecb5
Revert changes to axis line drawing
hannahker Dec 5, 2022
2b951fd
Merge branch 'master' into mult-axis-formatting
hannahker Dec 5, 2022
f97f27c
Properly account for axis title positioning
hannahker Dec 9, 2022
de4dd05
Fix syntax and add updated mocks
hannahker Dec 9, 2022
38d3e71
Make sure that the axes redraw with dragbox still works
hannahker Dec 9, 2022
683d0f3
Account for case where title fits inside axis labels
hannahker Dec 9, 2022
eca5d4f
Simplify syntax with feedback
hannahker Dec 14, 2022
b84f89d
Update baselines with smaller padding
hannahker Dec 15, 2022
5f2240f
Add jasmine test for axis positioning
hannahker Dec 15, 2022
81b661f
Fix syntax
hannahker Dec 15, 2022
ec1f855
Simplify function call
hannahker Dec 16, 2022
e56020d
Redraw if needed based on shift param
hannahker Dec 19, 2022
b8bfab5
Add baseline for new mock and fix syntax
hannahker Dec 19, 2022
b41678c
Merge branch 'master' into mult-axis-formatting
hannahker Dec 20, 2022
6064e5a
Rename redraw function and remove export
hannahker Dec 20, 2022
085c2b7
Adjust API to have both autoshift and shift params
hannahker Dec 21, 2022
49ddf46
Update plot schema and baseline
hannahker Dec 21, 2022
a45eca6
Adjust logic for default setting
hannahker Dec 21, 2022
56605bc
Adjust text for docs
hannahker Dec 21, 2022
5506ed4
Improve documentation and simplify syntax
hannahker Dec 21, 2022
61c2889
set ax._fullDepth only when needed
archmoj Dec 21, 2022
d06bc6d
simplify autoshift checks
archmoj Dec 21, 2022
bab8020
adjust description keys and values
archmoj Dec 21, 2022
09b5d0c
no shift when zero
archmoj Dec 21, 2022
b89f59a
do not add autoshift and shift to xaxis for now
archmoj Dec 21, 2022
948809b
adjust asterisks for new attributes shift & autoshift
archmoj Dec 21, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/components/titles/index.js
Expand Up @@ -202,6 +202,8 @@ function draw(gd, titleClass, options) {
}
});
shift = Math.min(maxshift, shift);
// Keeping track of this for calculation of full axis size if needed
cont._titleScoot = Math.abs(shift);
}

if(shift > 0 || maxshift < 0) {
Expand Down
3 changes: 3 additions & 0 deletions src/plot_api/subroutines.js
Expand Up @@ -240,6 +240,9 @@ function lsInner(gd) {
}

function yLinePathFree(x) {
if(ya._shift !== undefined) {
x += ya._shift;
}
hannahker marked this conversation as resolved.
Show resolved Hide resolved
hannahker marked this conversation as resolved.
Show resolved Hide resolved
return 'M' + x + ',' + ya._offset + 'v' + ya._length;
}

Expand Down
127 changes: 107 additions & 20 deletions src/plots/cartesian/axes.js
Expand Up @@ -2248,13 +2248,32 @@ axes.draw = function(gd, arg, opts) {

var axList = (!arg || arg === 'redraw') ? axes.listIds(gd) : arg;

var fullAxList = axes.list(gd);
// Get the list of the overlaying axis for all 'shift' axes
var overlayingShiftedAx = fullAxList.filter(function(ax) {
return ax.autoshift === true;
}).map(function(ax) {
return ax.overlaying;
});


var axShifts = {'false': {'left': 0, 'right': 0}};

return Lib.syncOrAsync(axList.map(function(axId) {
return function() {
if(!axId) return;

var ax = axes.getFromId(gd, axId);

if(!opts) opts = {};
opts.axShifts = axShifts;
opts.overlayingShiftedAx = overlayingShiftedAx;

var axDone = axes.drawOne(gd, ax, opts);

if(ax._shiftPusher) {
incrementShift(ax, ax._fullDepth, axShifts, true);
}
ax._r = ax.range.slice();
ax._rl = Lib.simpleMap(ax._r, ax.r2l);

Expand Down Expand Up @@ -2293,6 +2312,9 @@ axes.draw = function(gd, arg, opts) {
axes.drawOne = function(gd, ax, opts) {
opts = opts || {};

var axShifts = opts.axShifts || {};
var overlayingShiftedAx = opts.overlayingShiftedAx || [];

var i, sp, plotinfo;

ax.setScale();
Expand All @@ -2306,15 +2328,36 @@ axes.drawOne = function(gd, ax, opts) {
// this happens when updating matched group with 'missing' axes
if(!mainPlotinfo) return;

ax._shiftPusher = ax.autoshift === true ||
overlayingShiftedAx.indexOf(ax._id) !== -1 ||
overlayingShiftedAx.indexOf(ax.overlaying) !== -1;
// An axis is also shifted by 1/2 of its own linewidth and inside tick length if applicable
// as well as its manually specified `shift` val if we're in the context of `autoshift`
if(ax._shiftPusher & ax.anchor === 'free') {
var selfPush = (ax.linewidth / 2 || 0);
if(ax.ticks === 'inside') {
selfPush += ax.ticklen;
}
incrementShift(ax, selfPush, axShifts, true);
incrementShift(ax, (ax.shift || 0), axShifts, false);
}

// Somewhat inelegant way of making sure that the shift value is only updated when the
// Axes.DrawOne() function is called from the right context. An issue when redrawing the
// axis as result of using the dragbox, for example.
if(opts.skipTitle !== true || ax._shift === undefined) ax._shift = setShiftVal(ax, axShifts);

ax._fullDepth = 0;
var mainAxLayer = mainPlotinfo[axLetter + 'axislayer'];
var mainLinePosition = ax._mainLinePosition;
var mainLinePositionShift = mainLinePosition += ax._shift;
var mainMirrorPosition = ax._mainMirrorPosition;

var vals = ax._vals = axes.calcTicks(ax);

// Add a couple of axis properties that should cause us to recreate
// elements. Used in d3 data function.
var axInfo = [ax.mirror, mainLinePosition, mainMirrorPosition].join('_');
var axInfo = [ax.mirror, mainLinePositionShift, mainMirrorPosition].join('_');
for(i = 0; i < vals.length; i++) {
vals[i].axInfo = axInfo;
}
Expand Down Expand Up @@ -2409,8 +2452,8 @@ axes.drawOne = function(gd, ax, opts) {
var minorTickSigns = axes.getTickSigns(ax, 'minor');

if(ax.ticks || (ax.minor && ax.minor.ticks)) {
var majorTickPath = axes.makeTickPath(ax, mainLinePosition, majorTickSigns[2]);
var minorTickPath = axes.makeTickPath(ax, mainLinePosition, minorTickSigns[2], { minor: true });
var majorTickPath = axes.makeTickPath(ax, mainLinePositionShift, majorTickSigns[2]);
var minorTickPath = axes.makeTickPath(ax, mainLinePositionShift, minorTickSigns[2], { minor: true });

var mirrorMajorTickPath;
var mirrorMinorTickPath;
Expand Down Expand Up @@ -2496,7 +2539,7 @@ axes.drawOne = function(gd, ax, opts) {
layer: mainAxLayer,
plotinfo: plotinfo,
transFn: transTickLabelFn,
labelFns: axes.makeLabelFns(ax, mainLinePosition)
labelFns: axes.makeLabelFns(ax, mainLinePositionShift)
});
});

Expand All @@ -2515,28 +2558,34 @@ axes.drawOne = function(gd, ax, opts) {
repositionOnUpdate: true,
secondary: true,
transFn: transTickFn,
labelFns: axes.makeLabelFns(ax, mainLinePosition + standoff * majorTickSigns[4])
labelFns: axes.makeLabelFns(ax, mainLinePositionShift + standoff * majorTickSigns[4])
});
});

seq.push(function() {
ax._depth = majorTickSigns[4] * (getLabelLevelBbox('tick2')[ax.side] - mainLinePosition);
ax._depth = majorTickSigns[4] * (getLabelLevelBbox('tick2')[ax.side] - mainLinePositionShift);

return drawDividers(gd, ax, {
vals: dividerVals,
layer: mainAxLayer,
path: axes.makeTickPath(ax, mainLinePosition, majorTickSigns[4], { len: ax._depth }),
path: axes.makeTickPath(ax, mainLinePositionShift, majorTickSigns[4], { len: ax._depth }),
transFn: transTickFn
});
});
} else if(ax.title.hasOwnProperty('standoff')) {
seq.push(function() {
ax._depth = majorTickSigns[4] * (getLabelLevelBbox()[ax.side] - mainLinePosition);
ax._depth = majorTickSigns[4] * (getLabelLevelBbox()[ax.side] - mainLinePositionShift);
});
}

var hasRangeSlider = Registry.getComponentMethod('rangeslider', 'isVisible')(ax);

if(!opts.skipTitle &&
!(hasRangeSlider && ax.side === 'bottom')
) {
seq.push(function() { return drawTitle(gd, ax); });
}

seq.push(function() {
var s = ax.side.charAt(0);
var sMirror = OPPOSITE_SIDE[ax.side].charAt(0);
Expand All @@ -2548,7 +2597,7 @@ axes.drawOne = function(gd, ax, opts) {
var mirrorPush;
var rangeSliderPush;

if(ax.automargin || hasRangeSlider) {
if(ax.automargin || hasRangeSlider || ax._shiftPusher) {
if(ax.type === 'multicategory') {
llbbox = getLabelLevelBbox('tick2');
} else {
Expand All @@ -2559,10 +2608,27 @@ axes.drawOne = function(gd, ax, opts) {
}
}

var axDepth = 0;
var titleDepth = 0;
if(ax._shiftPusher) {
axDepth = Math.max(
outsideTickLen,
llbbox.height > 0 ? (s === 'l' ? pos - llbbox.left : llbbox.right - pos) : 0
);
if(ax.title.text !== fullLayout._dfltTitle[axLetter]) {
titleDepth = (ax._titleStandoff || 0) + (ax._titleScoot || 0);
if(s === 'l') {
titleDepth += approxTitleDepth(ax);
}
hannahker marked this conversation as resolved.
Show resolved Hide resolved
}

ax._fullDepth = Math.max(axDepth, titleDepth);
}

if(ax.automargin) {
push = {x: 0, y: 0, r: 0, l: 0, t: 0, b: 0};
var domainIndices = [0, 1];

var shift = typeof ax._shift === 'number' ? ax._shift : 0;
if(axLetter === 'x') {
if(s === 'b') {
push[s] = ax._depth;
Expand All @@ -2585,9 +2651,11 @@ axes.drawOne = function(gd, ax, opts) {
}
} else {
if(s === 'l') {
push[s] = ax._depth = Math.max(llbbox.height > 0 ? pos - llbbox.left : 0, outsideTickLen);
ax._depth = Math.max(llbbox.height > 0 ? pos - llbbox.left : 0, outsideTickLen);
push[s] = ax._depth - shift;
} else {
push[s] = ax._depth = Math.max(llbbox.height > 0 ? llbbox.right - pos : 0, outsideTickLen);
ax._depth = Math.max(llbbox.height > 0 ? llbbox.right - pos : 0, outsideTickLen);
push[s] = ax._depth + shift;
domainIndices.reverse();
}

Expand Down Expand Up @@ -2626,7 +2694,6 @@ axes.drawOne = function(gd, ax, opts) {
}
}
}

if(hasRangeSlider) {
rangeSliderPush = Registry.getComponentMethod('rangeslider', 'autoMarginOpts')(gd, ax);
}
Expand All @@ -2641,12 +2708,6 @@ axes.drawOne = function(gd, ax, opts) {
Plots.autoMargin(gd, rangeSliderAutoMarginID(ax), rangeSliderPush);
});

if(!opts.skipTitle &&
!(hasRangeSlider && ax.side === 'bottom')
) {
seq.push(function() { return drawTitle(gd, ax); });
}

return Lib.syncOrAsync(seq);
};

Expand Down Expand Up @@ -3779,7 +3840,7 @@ axes.getPxPosition = function(gd, ax) {
};
} else if(axLetter === 'y') {
anchorAxis = {
_offset: gs.l + (ax.position || 0) * gs.w,
_offset: gs.l + (ax.position || 0) * gs.w + ax._shift,
_length: 0
};
}
Expand Down Expand Up @@ -3902,6 +3963,8 @@ function drawTitle(gd, ax) {
}
}

ax._titleStandoff = titleStandoff;

return Titles.draw(gd, axId + 'title', {
propContainer: ax,
propName: ax._name + '.title.text',
Expand Down Expand Up @@ -4211,3 +4274,27 @@ function hideCounterAxisInsideTickLabels(ax, opts) {
}
}
}

function incrementShift(ax, shiftVal, axShifts, normalize) {
// Need to set 'overlay' for anchored axis
var overlay = ((ax.anchor !== 'free') && ((ax.overlaying === undefined) || (ax.overlaying === false))) ? ax._id : ax.overlaying;
var shiftValAdj;
if(normalize) {
shiftValAdj = ax.side === 'right' ? shiftVal : -shiftVal;
} else {
shiftValAdj = shiftVal;
}
if(!(overlay in axShifts)) {
axShifts[overlay] = {};
}
if(!(ax.side in axShifts[overlay])) {
axShifts[overlay][ax.side] = 0;
}
axShifts[overlay][ax.side] += shiftValAdj;
}

function setShiftVal(ax, axShifts) {
return ax.autoshift === true ?
axShifts[ax.overlaying][ax.side] :
(ax.shift || 0);
}
2 changes: 0 additions & 2 deletions src/plots/cartesian/axis_defaults.js
Expand Up @@ -163,8 +163,6 @@ module.exports = function handleAxisDefaults(containerIn, containerOut, coerce,
// mirror
if(containerOut.showline || containerOut.ticks) coerce('mirror');

if(options.automargin) coerce('automargin');

var isMultiCategory = axType === 'multicategory';

if(!options.noTickson &&
Expand Down
3 changes: 3 additions & 0 deletions src/plots/cartesian/dragbox.js
Expand Up @@ -88,6 +88,9 @@ function makeDragBox(gd, plotinfo, x, y, w, h, ns, ew) {
var scaleX;
var scaleY;

// offset the x location of the box if needed
x += plotinfo.yaxis._shift;

function recomputeAxisLists() {
xa0 = plotinfo.xaxis;
ya0 = plotinfo.yaxis;
Expand Down
22 changes: 22 additions & 0 deletions src/plots/cartesian/layout_attributes.js
Expand Up @@ -1003,6 +1003,28 @@ module.exports = {
'Only has an effect if `anchor` is set to *free*.'
].join(' ')
},
autoshift: {
valType: 'boolean',
dflt: false,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens / should happen if one sets shift to zero?
Can't we use zero as default instead of false?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shift=0 will cause the axis to directly overlap the one that is anchored to the graph on the same size, which is the same as the default shift=false. I do think shift=false is more intuitive as the default as it's likely that most people will use shift=true -- the numeric values are for rarer cases with really fine-tuned positioning needs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Numbers and “auto” (rather than true) is a pattern we’ve used several other places

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the default off is "false", would it be clearer for the 'on' to be "true"? But happy to switch to "auto" if that's better aligned with the existing standard.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, the idea would be to not allow true and false, just numbers and 'auto', but with a default of 0.

Numbers plus 'auto' is what we do for example with tickangle (valType: 'angle' is a number with special behavior to wrap larger values into [-180, 180]), and font size and a couple of other places.

On the other hand, in all those cases 'auto' is the default, whereas here we want 0 to be the default. As you have it now, this requires shift to have valType: 'any' which isn't great, it really should be valType: 'number'. We could add extras like we have for flaglist attributes (actually I thought we already had this, but apparently not), or a more targeted option like autoOk: true to just allow 'auto'. If we do this though we need matching logic in the plotly.py NumberValidator

That's a bit annoying, though I suspect unless we do that there's a subtle bug with the above number-with-default-auto cases: if you set a non-auto value in a template you won't be able to get back to auto later. Or even just if you've set a value in your figure and you want to unset it later, or explicitly affirm the value 'auto'.

I do like the idea of shift='auto' or shift=<some number>, I think it's easy to document, intuitive to read if not guessable without the docs, and compact. But if we want a solution that doesn't involve modifying the behavior of coerce we can either:

  • leave it as valType: 'any' for the moment, and change it in a follow-up PR when we have more time to devote to coerce and the plotly.py validators
  • make two attributes, shift with valType: 'number' and autoshift with valType: 'boolean' (and if true we don't coerce shift).

@hannahker @archmoj thoughts?

We do need to update AngleValidator regardless - it doesn't support arrayOk as we discussed recently on slack and also seems to not handle an explicit 'auto' (cc @nicolaskruchten)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should possibly make this attribute an object then provide various current/future options inside it e.g.

shift: {
  enabled: boolean,
  value: number
}

Copy link
Contributor Author

@hannahker hannahker Dec 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexcjohnson @archmoj so we've settled on an API with two params:

  1. shiftauto: boolean where you turn on/off the automatic repositioning of axes that would be overlapping. Default = 'false'
  2. shiftval: numeric where you indicate the number of pixels that you want to move the axis from where it would otherwise be. Default = 0.

Is this correct?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(already posted this on slack, but repeating so it's visible to all)
OK after discussing further, there's a nice reason to make these separate attributes: so when the axis is auto-shifted, you can use the shift parameter to control the extra spacing between subsequent axes.

  • autoshift: boolean, default false
  • shift: number, no dflt defined in the attribute itself, but the dynamic (and described in the attribute description) default is 0 when autoshift===false and 3 (yes +/-3 I guess, depending on the side) if autoshift===true

editType: 'plot',
description: [
'Automatically reposition the axis to avoid',
'overlap with other axes with the same `overlaying` value.',
'This repositioning will account for any `shift` amount applied to other',
'axes on the same side with `autoshift=true`.',
'Only has an effect if `anchor` is set to *free*.',
].join(' ')
},
shift: {
valType: 'number',
editType: 'plot',
description: [
'Moves the axis a given number of pixels from where it would have been otherwise.',
'If `autoshift=true`, then this defaults to a padding of `-3` if `side=left`',
'and `+3` if `side=right`. Defaults to `0` if `autoshift=false`.',
archmoj marked this conversation as resolved.
Show resolved Hide resolved
'Only has an effect if `anchor` is set to *free*.'
].join(' ')
},
categoryorder: {
valType: 'enumerated',
values: [
Expand Down
15 changes: 14 additions & 1 deletion src/plots/cartesian/layout_defaults.js
Expand Up @@ -266,11 +266,24 @@ module.exports = function supplyLayoutDefaults(layoutIn, layoutOut, fullData) {
delete axLayoutOut.spikesnap;
}

// If it exists, the the domain of the axis for the anchor of the overlaying axis
var overlayingAxis = id2name(axLayoutIn.overlaying);
var overlayingAnchorDomain = [0, 1];

if(layoutOut[overlayingAxis] !== undefined) {
var overlayingAnchor = id2name(layoutOut[overlayingAxis].anchor);
if(layoutOut[overlayingAnchor] !== undefined) {
overlayingAnchorDomain = layoutOut[overlayingAnchor].domain;
}
}

handlePositionDefaults(axLayoutIn, axLayoutOut, coerce, {
letter: axLetter,
counterAxes: counterAxes[axLetter],
overlayableAxes: getOverlayableAxes(axLetter, axName),
grid: layoutOut.grid
grid: layoutOut.grid,
overlayingDomain: overlayingAnchorDomain

});

coerce('title.standoff');
Expand Down