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

Axis default positions #3893

Merged
merged 1 commit into from
Feb 12, 2017
Merged

Axis default positions #3893

merged 1 commit into from
Feb 12, 2017

Conversation

etimberg
Copy link
Member

This PR contains 2 changes.

  1. Removed the references to Chart.Controller from the JSDoc comments
  2. When creating axes, we now define the default position for axes. If the position option of the axis does not match the actual location, we change the position. This prevents incorrectly formed charts and has been heavily requested.

Example:

let options = {
  scales: {
    xAxes: [{
      type: 'linear',
      position: 'right' // will be changed to 'bottom' since 'right' is not an x axis
    }],
    yScales: [{
      position: 'bottom' // will be changed to 'left' since 'bottom' is not a y axis position
    }]
  }
};

@etimberg etimberg added this to the Version 2.6 milestone Feb 11, 2017
@etimberg etimberg force-pushed the axis-default-positions branch 2 times, most recently from 2befa84 to b6ffd5b Compare February 11, 2017 15:22
Copy link
Member

@simonbrunel simonbrunel left a comment

Choose a reason for hiding this comment

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

Looks good, is unit tests pertinent for this change?

@@ -240,6 +249,11 @@ module.exports = function(Chart) {
return;
}

if (positionIsHorizontal(scaleOptions.position) !==
Copy link
Member

Choose a reason for hiding this comment

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

[minor] can we keep the condition on the same line, quite confusing when reading :) ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I will make that change

@etimberg etimberg force-pushed the axis-default-positions branch 2 times, most recently from ba4b76f to 0598832 Compare February 11, 2017 15:38
@etimberg
Copy link
Member Author

I added a unit test to cover this.

@etimberg etimberg force-pushed the axis-default-positions branch from 0598832 to 202e439 Compare February 11, 2017 18:42
@simonbrunel
Copy link
Member

+1 to merge! what's the reason unit tests values required adjustments?

@etimberg
Copy link
Member Author

Previously, there were 3 axes in the chart in that test (1 on the x, 1 on the y at position 'left' and one on the y at position 'bottom' so I cleaned it up to have only 2 and mimic the old fitting but the sizes changed slightly

@etimberg etimberg merged commit 8dafbc0 into master Feb 12, 2017
@etimberg etimberg deleted the axis-default-positions branch February 12, 2017 16:23
@skhan02
Copy link

skhan02 commented May 10, 2019

@etimberg any idea on how i can get the axis labels to render with the x-axis instead of top or bottom?

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

Successfully merging this pull request may close these issues.

4 participants