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

Add additional options for automargin property #6193

Merged

Conversation

nickmelnikov82
Copy link
Member

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.

'top left', 'top width', 'top right',
'left height', 'right height',
'bottom left', 'bottom width', 'bottom right',
],
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

flaglist does not support boolean parameters

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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...

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed the schema.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

💃 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.

@nickmelnikov82 nickmelnikov82 force-pushed the additional-options-for-automargin-property branch 2 times, most recently from 85cbe5d to fa888ab Compare May 25, 2022 08:34
@nickmelnikov82 nickmelnikov82 force-pushed the additional-options-for-automargin-property branch from fa888ab to 9dfdb91 Compare May 25, 2022 08:48
if(typeof v !== 'string') {
propOut.set(dflt);
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please elaborate why this change is needed?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

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

keepSelectedAutoMargin(ax.automargin, push);
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

draftlogs/6193_add.md Outdated Show resolved Hide resolved

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

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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])

@@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

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

How about rangeSliderPush? Should we filter that one too?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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."

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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
Copy link
Collaborator

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

@nickmelnikov82 nickmelnikov82 force-pushed the additional-options-for-automargin-property branch 3 times, most recently from aa8fb3d to e26eb02 Compare May 27, 2022 14:28
@nickmelnikov82 nickmelnikov82 force-pushed the additional-options-for-automargin-property branch from e26eb02 to eed9bbf Compare May 27, 2022 15:13
@@ -2636,6 +2649,25 @@ axes.drawOne = function(gd, ax, opts) {
return Lib.syncOrAsync(seq);
};

function filterPush(push, automargin) {
if(!push) return push;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if(!push) return push;
if(!push) return;

else delete push[key];
}
});
return push;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return push;

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

archmoj commented May 30, 2022

Thanks very much for the PR 💃

@archmoj archmoj merged commit f92f786 into plotly:master May 30, 2022
@nickmelnikov82 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
…, "bottom", "width" and "height"

to control the direction of automargin on cartesian axes (plotly/plotly.js#6193)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature something new
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants