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

image: constrain axes to domain, apply defaults whenever an image is present #4313

Merged
merged 6 commits into from
Oct 29, 2019

Conversation

antoinerg
Copy link
Contributor

@antoinerg antoinerg commented Oct 28, 2019

Following #4307 (comment), this PR adds a smart default layout to constrain x and y axes to the domain if there is an image present.

The notion of a "smart default layout" is changed slightly in this PR for image. Now, the layout defaults will be applied to a subplot's axes whenever an image is present in the subplot. Adding a scatter on top will not affect the defaults. This is clearly tested in a very explicit (and probably redundant) series of jasmine tests.

@antoinerg antoinerg changed the title image: smart default layout to constrain y axis to domain image: constrain y axis to domain, apply defaults whenever an image is present Oct 29, 2019
@antoinerg antoinerg changed the title image: constrain y axis to domain, apply defaults whenever an image is present image: constrain axes to domain, apply defaults whenever an image is present Oct 29, 2019
if(axLetter === 'y' && !axLayoutIn.hasOwnProperty('scaleanchor') && axHasImage[axName]) {
scaleanchorDflt = axLayoutOut.anchor;
}

var constrainDflt = null;
var constrainDflt;
Copy link
Contributor

@etpinard etpinard Oct 29, 2019

Choose a reason for hiding this comment

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

I think we need to reset to null as we're in a loop:

var axNames = ['xaxis', 'yaxis']

for(var i = 0; i < axNames.length; i++) {
  var dflt;
  if(axNames[i] === 'xaxis') {
    dflt = 'NEW DEFAULT';
  }
  console.log(dflt)
}

prints NEW DEFAULT twice, whereas

var axNames = ['xaxis', 'yaxis']

for(var i = 0; i < axNames.length; i++) {
  var dflt = null;
  if(axNames[i] === 'xaxis') {
    dflt = 'NEW DEFAULT';
  }
  console.log(dflt)
}

print NEW DEFAULT and then null.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right but setting it to null will prevent var constrain = coerce('constrain', constrainDflt); from getting the default value. This can be seen in the previous commit d1eca2c for which test-image fails. Any suggestion/simple fix for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Setting it to undefined make the tests pass but the linter complains :\

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The following pass all the tests (including linter). Would that be 👌 ?

        var scaleanchorDflt;
        if(axLetter === 'y' && !axLayoutIn.hasOwnProperty('scaleanchor') && axHasImage[axName]) {
            scaleanchorDflt = axLayoutOut.anchor;
        } else {scaleanchorDflt = undefined;}

        var constrainDflt;
        if(!axLayoutIn.hasOwnProperty('constrain') && axHasImage[axName]) {
            constrainDflt = 'domain';
        } else {constrainDflt = undefined;}

Copy link
Contributor

Choose a reason for hiding this comment

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

What about:

        var scaleanchorDflt = undefined;
        if(axLetter === 'y' && !axLayoutIn.hasOwnProperty('scaleanchor') && axHasImage[axName]) {
            scaleanchorDflt = axLayoutOut.anchor;
        }

        var constrainDflt = undefined;
        if(!axLayoutIn.hasOwnProperty('constrain') && axHasImage[axName]) {
            constrainDflt = 'domain';
        }

??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screenshot_2019-10-29_14-02-21

Copy link
Contributor

Choose a reason for hiding this comment

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

Ha that's annoying. Your latest is fine 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 29cf98f

Copy link
Contributor

Choose a reason for hiding this comment

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

by latest I mean:

        var scaleanchorDflt;
        if(axLetter === 'y' && !axLayoutIn.hasOwnProperty('scaleanchor') && axHasImage[axName]) {
            scaleanchorDflt = axLayoutOut.anchor;
        } else {
            scaleanchorDflt = undefined;
        }

        var constrainDflt;
        if(!axLayoutIn.hasOwnProperty('constrain') && axHasImage[axName]) {
            constrainDflt = 'domain';
        } else {
            constrainDflt = undefined;
        }

@etpinard
Copy link
Contributor

Awesome - 💃 let's get this in #4307

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants