Skip to content

WIP Scale options as an object instead of an array#6626

Closed
etimberg wants to merge 9 commits intochartjs:masterfrom
etimberg:scale-options-object
Closed

WIP Scale options as an object instead of an array#6626
etimberg wants to merge 9 commits intochartjs:masterfrom
etimberg:scale-options-object

Conversation

@etimberg
Copy link
Member

I want to get some feedback on this before I go any further.

This PR changes the axis options format to one that is easier to work with. The challenges are that we can't do as many default actions for X vs Y axes. I kept some basic functionality by assuming that axes that have an ID starting with "x" are horizontal.


options: {
	scales: {
		xAxes: [{
                        id: 'x',
			display: true,
			scaleLabel: {
				display: true,
				labelString: 'Month'
			}
		}],
		yAxes: [{
                        id: "y",
			display: true,
			scaleLabel: {
				display: true,
				labelString: 'Value'
			}
		}]
	}
}

to

options: {
	scales: {
		x: {
			display: true,
			scaleLabel: {
				display: true,
				labelString: 'Month'
			}
		},
		y: {
			display: true,
			scaleLabel: {
				display: true,
				labelString: 'Value'
			}
		}
	}
}

To Do

  • Fixture based tests
  • Documentation

etimberg and others added 7 commits October 27, 2019 18:50
* Updated all chart type defaults
* Throw errors when axis type or position are not specified
* Avoid raising unnecessary errors when merging options into the default configs
@etimberg etimberg requested review from benmccann and kurkle October 27, 2019 22:56
@kurkle
Copy link
Member

kurkle commented Oct 27, 2019

I like it. How about allowing position to be option and fall back to "starts with x"?

@etimberg
Copy link
Member Author

Yup, position as an option is still available. Just that previously determining the default was easy since we knew already if the axis was X or Y.

One other downside I realized is that if one creates new axes x1 and y1, they'll end up with 4 because the charts have defaults for x and y IDs. Previously the ID didn't really matter. I can see that causing complaints and/or lots of issues

@benmccann
Copy link
Contributor

What's the primary goal here? I'll probably have some thoughts, but want to make sure I'm not going in a different direction

@kurkle
Copy link
Member

kurkle commented Oct 28, 2019

I think the axis property for scale, proposed by @benmccann in #6576 (comment) could help here too?

@etimberg
Copy link
Member Author

@benmccann the goal here was to try and simplify the axis option code by changing the format of the config object.

benmccann
benmccann previously approved these changes Oct 28, 2019
Copy link
Contributor

@benmccann benmccann left a comment

Choose a reason for hiding this comment

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

I like it

One thing I was thinking is that we should probably have a migration guide that we update each time we make a breaking change. I think it will be easier to do as we go than to have to go back and do it all at the end. And easier for the person originally making the change than for one person to have to do it all

id: 'firstYScaleID'
}]
}
}
Copy link
Member

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 figure out how to default to first configured axis instead of x/y

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed. I think the thing that may sink this attempt is taht one cannot remove the x and y axis IDs because of how we merge the config.

Copy link
Member

Choose a reason for hiding this comment

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

I think we can map defaults.x to firstX and defaults.y to firstY in merge, but might have to rewrite the merge totally.

if (meta.yAxisID === null || !(meta.yAxisID in scales) || dataset.yAxisID) {
meta.yAxisID = dataset.yAxisID || scalesOpts.yAxes[0].id;
meta.yAxisID = dataset.yAxisID || scalesOpts.y.id;
}
Copy link
Member

Choose a reason for hiding this comment

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

We could do something like:

var firstX = Object.keys(scales).filter(key => scales[key].position === 'top' || scales[key].position === 'bottom').shift();
var firstY = Object.keys(scales).filter(key => scales[key].position === 'left' || scales[key].position === 'right').shift();

if (meta.xAxisID === null || !(meta.xAxisID in scales) || dataset.xAxisID) {
	meta.xAxisID = dataset.xAxisID || firstX;
}
if (meta.yAxisID === null || !(meta.yAxisID in scales) || dataset.yAxisID) {
	meta.yAxisID = dataset.yAxisID || firstY;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea, let me see how that goes

@benmccann
Copy link
Contributor

@etimberg this one will need to be rebased

@kurkle
Copy link
Member

kurkle commented Nov 22, 2019

Merged in #6773

@kurkle kurkle closed this Nov 22, 2019
@etimberg etimberg deleted the scale-options-object branch October 17, 2020 14:23
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.

3 participants