Convert axis options from arrays to objects #6773
Conversation
* 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
That sounds fine to me. I don't think we need to update them all this PR. Just FYI, I fixed about half the fixtures doing a command-line search and replace with I just realized that I forgot to update the docs. It looks like there weren't too many references there and you may have already gotten them, but you may want to do a search |
benmccann
left a comment
benmccann
left a comment
There was a problem hiding this comment.
This looks good to me. It's getting hard to review now that we're up to 200 files, but I think we could merge it and clean up anything we missed over time since it looks pretty close
etimberg
left a comment
etimberg
left a comment
There was a problem hiding this comment.
Looks great. Thanks for taking my MR and making it great! Just one minor comment re a test
| defaults.line.spanGaps = false; | ||
| }); | ||
|
|
||
| it('should override axis positions that are incorrect', function() { |
There was a problem hiding this comment.
can we add this back at all? I realize this might have been removed from my original MR but I think we can support it still in this form
There was a problem hiding this comment.
Good catch, added the test back!
benmccann
left a comment
benmccann
left a comment
There was a problem hiding this comment.
lgtm. just some minor notes. I'm okay if we prefer to do anything else in a follow up
| ### Axis ID | ||
| The properties `dataset.xAxisID` or `dataset.yAxisID` have to match the scale properties `scales.xAxes.id` or `scales.yAxes.id`. This is especially needed if multi-axes charts are used. | ||
|
|
||
| The properties `dataset.xAxisID` or `dataset.yAxisID` have to match to `scales` property. This is especially needed if multi-axes charts are used. |
There was a problem hiding this comment.
It might sound more natural to say "have to match a property of scales"
| scales: { | ||
| yAxes: [{ | ||
| id: 'first-y-axis', | ||
| 'first-y-axis': { |
There was a problem hiding this comment.
I'm assuming this syntax works though it's a bit awkward as an example. Maybe firstYAxis would be easier since it wouldn't require the single quotes
| }, { | ||
| type: 'linear', // only linear but allow scale type registration. This allows extensions to exist solely for log scale for instance | ||
| }, | ||
| y1: { |
There was a problem hiding this comment.
y and y1 is not quite what I'd expect. Would y1 and y2 work? (same in the other samples)
| Object.entries(options.scales).map(function(entry) { | ||
| var axisID = entry[0]; | ||
| var axisOptions = entry[1]; | ||
| var isRadial = axisID.charAt(0).toLowerCase === 'r'; |
There was a problem hiding this comment.
maybe add a TODO here or document that the first character must be x, y, or r?
3 participants
Another step further from #6770, completely rewritten scale option resolution.
axis(x/y/r) instead ofscale.idI had to add
offset: falseto bunch of bar tests, those dataset defaults were not applied before if not using default scales.