Add additional options for automargin property #6193

nickmelnikov82 · 2022-05-19T16:12:58Z

Feature: Add new options "width" and "height" for the automargin property, which would limit the automargin calculation to one dimension. This way we could make sure the labels are never cut off and that there isn't any whitespace on the left or right border of the figure.

src/plots/cartesian/axes.js

alexcjohnson · 2022-05-21T21:12:16Z

src/plots/cartesian/layout_attributes.js

+            'top left', 'top width', 'top right',
+            'left height', 'right height',
+            'bottom left', 'bottom width', 'bottom right',
+        ],


Let's use a flaglist:

valType: 'flaglist', flags: ['height', 'width', 'left', 'right', 'top', 'bottom'], extras: [true, false]

That way the order doesn't matter (you use + - bottom+left and left+bottom are both valid), and users could even do bottom+left+right which would mean the same thing as bottom+width

Are there pre-existing instances of "flaglist or boolean" in the schema? I want to make sure the Python codegen will not choke on this.

flaglist does not support boolean parameters

plotly.js/src/lib/coerce.js

Line 218 in b194d8c

if(typeof v !== 'string') {

OK that's pretty good evidence that we don't currently have any flaglists with non-string extras 😉
We could allow this in coerce pretty easily by pushing the extras test up above that non-string short-circuit.

@nicolaskruchten your concern is warranted, plotly.py also includes the string requirement up front. But I really do think this attribute would be better as a flaglist, I'll be happy to modify the Python validator to allow non-string extras.

I suppose the alternative would be to make this a new attribute like automargindirections and leave automargin as a boolean, but that seems a bit cumbersome...

I'm fine with tweaking Plotly.py for this 👍

Changed the schema.

OK that's pretty good evidence that we don't currently have any flaglists with non-string extras wink We could allow this in coerce pretty easily by pushing the extras test up above that non-string short-circuit.

@nicolaskruchten your concern is warranted, plotly.py also includes the string requirement up front. But I really do think this attribute would be better as a flaglist, I'll be happy to modify the Python validator to allow non-string extras.

I suppose the alternative would be to make this a new attribute like automargindirections and leave automargin as a boolean, but that seems a bit cumbersome...

Having a separate attribute could be safer for Python, R, Scala, etc. APIs.
It would be nice to possibly test this at plotly.py level before merging this PR.

I'm working on a plotly.py PR right now... will post shortly.

src/plots/cartesian/axes.js

alexcjohnson

💃 This looks great to me. It'll just need a draftlog entry, @archmoj anything from your side?

I'll make the required plotly.py change for non-string flaglist extras myself.

archmoj · 2022-05-25T13:36:28Z

src/lib/coerce.js

+            if(typeof v !== 'string') {
+                propOut.set(dflt);
+                return;
+            }


Could you please elaborate why this change is needed?

Just reversing the order of short-circuits to support non-string extras #6193 (comment)

As @alexcjohnson wrote:

OK that's pretty good evidence that we don't currently have any flaglists with non-string extras 😉 We could allow this in coerce pretty easily by pushing the extras test up above that non-string short-circuit.

Without this, the flaglist coerce function does not pass non-string values.

archmoj · 2022-05-25T15:40:21Z

src/plots/cartesian/axes.js

@@ -2622,6 +2622,8 @@ axes.drawOne = function(gd, ax, opts) {
            rangeSliderPush = Registry.getComponentMethod('rangeslider', 'autoMarginOpts')(gd, ax);
        }

+        keepSelectedAutoMargin(ax.automargin, push);


So keepSelectedAutoMargin mutates the push variable.
It was rather difficult to figure out what this step does here.
How about refactoring like this:

if(typeof automargin === 'string') filterPush(push, ax.automargin);

On another note, wondering if mutating push should be performed earlier in the process so that the value in mirrorPush would be accurate?

I don't see the point of using a mirror if the result would still be cut off. So there is no need to filter the mirrorPush.

Actually I think it's correct to filter mirrorPush as well. The whole mirrorPush construction is inside the if(ax.automargin) block - implying that if you disable automargin, we're not going to push the margins for mirror axes regardless of whether they end up cut off. Therefore you should be able to choose which directions this applies to just as you can the regular axis automargin.

src/plots/cartesian/axes.js

draftlogs/6193_add.md

archmoj · 2022-05-26T15:37:26Z

src/plots/cartesian/axes.js

+
+    Object.keys(MARGIN_MAPPING).forEach(function(key) {
+        if(automargin.indexOf(key) !== -1) {
+            MARGIN_MAPPING[key].forEach(function(item) {


Can't we simply use one concact here instead of the inner loop and push?

actually perhaps even better: make keepMargin an object and set keepMargin[item] = 1, then the test below doesn't need indexOf, just if(!keepMargin[key])

archmoj · 2022-05-26T15:54:17Z

src/plots/cartesian/axes.js

@@ -2622,6 +2630,11 @@ axes.drawOne = function(gd, ax, opts) {
            rangeSliderPush = Registry.getComponentMethod('rangeslider', 'autoMarginOpts')(gd, ax);
        }

+        if(typeof ax.automargin === 'string') {
+            filterPush(push, ax.automargin);
+            filterPush(mirrorPush, ax.automargin);


How about rangeSliderPush? Should we filter that one too?

ha @nickmelnikov82 asked me the same question, my answer:

that one isn’t controlled by ax.automargin so I think no

There are others that aren't even in this section of code, like legends and colorbars. ax.automargin is only about "are the axes themselves allowed to increase the margins."

I wonder if we want to implement layout.automargindirections and inherit from it instead?

Oh that's interesting... in principle I like this, one place to control all of this rather than requiring you to set it in each axis separately. But it opens a can of worms: by default axes have automargin: false while colorbars and legends (and rangesliders) by default DO increase the margins, and putting this at the top level would imply that it applies to everything. So it would need a default value like 'legacy' or something that gets inherited differently by axes and other objects, and then we would need to extend support for disabling automargin to all these other objects as well.

All that to say: it's a good idea, but it's going to be a bunch of work, so let's not do it now. What we're doing in this PR would be fully compatible with adding that sometime in the future.

alexcjohnson · 2022-05-26T16:27:25Z

Created plotly/plotly.py#3749 to allow non-string flaglist extras on the Python side

archmoj · 2022-05-27T15:20:39Z

src/plots/cartesian/axes.js

@@ -2636,6 +2649,25 @@ axes.drawOne = function(gd, ax, opts) {
    return Lib.syncOrAsync(seq);
 };

+function filterPush(push, automargin) {
+    if(!push) return push;


Suggested change

if(!push) return push;

if(!push) return;

archmoj · 2022-05-27T15:21:04Z

src/plots/cartesian/axes.js

+            else delete push[key];
+        }
+    });
+    return push;


Suggested change

return push;

archmoj · 2022-05-30T12:45:04Z

Thanks very much for the PR 💃

…, "bottom", "width" and "height" to control the direction of automargin on cartesian axes (plotly/plotly.js#6193)

nickmelnikov82 added 2 commits May 19, 2022 19:05

add additional automargin options

25a54a2

update plot-schema diff

0ed3602

archmoj added feature something new status: reviewable labels May 20, 2022

alexcjohnson reviewed May 21, 2022

View reviewed changes

src/plots/cartesian/axes.js Outdated Show resolved Hide resolved

alexcjohnson reviewed May 21, 2022

View reviewed changes

src/plots/cartesian/axes.js Outdated Show resolved Hide resolved

nickmelnikov82 added 3 commits May 23, 2022 15:27

added IE11 compatibility

0702b78

use flaglist with non-string extra parameters

35268c7

update plot-schema diff

57452c2

alexcjohnson approved these changes May 24, 2022

View reviewed changes

nickmelnikov82 force-pushed the additional-options-for-automargin-property branch 2 times, most recently from 85cbe5d to fa888ab Compare May 25, 2022 08:34

add changelog entry

9dfdb91

nickmelnikov82 force-pushed the additional-options-for-automargin-property branch from fa888ab to 9dfdb91 Compare May 25, 2022 08:48

archmoj reviewed May 25, 2022

View reviewed changes

src/plots/cartesian/axes.js Outdated Show resolved Hide resolved

archmoj reviewed May 25, 2022

View reviewed changes

draftlogs/6193_add.md Outdated Show resolved Hide resolved

archmoj added the status: has TODOs label May 26, 2022

nickmelnikov82 added 2 commits May 26, 2022 17:25

refactor of the push filtering function

cf26237

add mirrorPush filtering

ee97962

archmoj reviewed May 26, 2022

View reviewed changes

alexcjohnson mentioned this pull request May 26, 2022

allow non-string extras in flaglist attributes plotly/plotly.py#3749

Merged

2 tasks

nickmelnikov82 force-pushed the additional-options-for-automargin-property branch 3 times, most recently from aa8fb3d to e26eb02 Compare May 27, 2022 14:28

minore codestyle fixes

eed9bbf

nickmelnikov82 force-pushed the additional-options-for-automargin-property branch from e26eb02 to eed9bbf Compare May 27, 2022 15:13

archmoj reviewed May 27, 2022

View reviewed changes

src/plots/cartesian/axes.js Outdated

else delete push[key];

}

});

return push;

Copy link

Contributor

archmoj May 27, 2022

Choose a reason for hiding this comment

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

Suggested change

return push;

removed redundant return data

ae1b087

nickmelnikov82 force-pushed the additional-options-for-automargin-property branch from a2d6fd5 to ae1b087 Compare May 27, 2022 15:44

archmoj removed the status: has TODOs label May 30, 2022

archmoj merged commit f92f786 into plotly:master May 30, 2022

nickmelnikov82 deleted the additional-options-for-automargin-property branch May 30, 2022 13:03

kMutagene added a commit to plotly/Plotly.NET that referenced this pull request Feb 6, 2023

plotlyjs v2.13: Add flaglist options including "left", "right", "top"…

5c7cadd

…, "bottom", "width" and "height" to control the direction of automargin on cartesian axes (plotly/plotly.js#6193)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add additional options for automargin property #6193

Add additional options for automargin property #6193

nickmelnikov82 commented May 19, 2022

alexcjohnson May 21, 2022

nicolaskruchten May 22, 2022

nickmelnikov82 May 23, 2022

alexcjohnson May 23, 2022

nicolaskruchten May 24, 2022

nickmelnikov82 May 24, 2022

archmoj May 26, 2022

alexcjohnson May 26, 2022

alexcjohnson left a comment

archmoj May 25, 2022

alexcjohnson May 25, 2022

nickmelnikov82 May 25, 2022

archmoj May 25, 2022

nickmelnikov82 May 26, 2022

alexcjohnson May 26, 2022

archmoj May 26, 2022

alexcjohnson May 26, 2022

archmoj May 26, 2022

alexcjohnson May 26, 2022

archmoj May 26, 2022

alexcjohnson May 26, 2022

alexcjohnson commented May 26, 2022

archmoj May 27, 2022

archmoj May 27, 2022

archmoj commented May 30, 2022

Add additional options for automargin property #6193

Add additional options for automargin property #6193

Conversation

nickmelnikov82 commented May 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcjohnson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcjohnson commented May 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

archmoj commented May 30, 2022