Replace horizontalBar with indexAxis: 'y' #7144

kurkle · 2020-02-22T17:02:28Z

Contains some auxiliary changes:

borderSkipped support for 'start' / 'end'
dataset default scales definable by i and v (i = base/index axis, v = value axis)
radial charts don't require scale to be r (use _cachedMeta.rScale)
_getIndexScaleId, _getIndexScale, _getValueScaleId, _getValueScale helpers removed

benmccann · 2020-02-22T17:18:57Z

You'll have to add a note to the migration guide. Also, lots of failing tests

etimberg · 2020-02-22T19:55:39Z

src/controllers/controller.bar.js

@@ -22,14 +23,14 @@ defaults.set('bar', {
 	},

 	scales: {
-		x: {
+		i: {


How do we detect the positions now?

i is replaced with the baseAxis and v with the other one. maybe more verbose names here would be good.

Maybe use index and value? (sort of makes me wonder if it should be indexAxis instead of baseAxis)

Changed to _index_, _value_ and indexAxis.
Now also reads the indexAxis from chart options directly.
Can have different indexAxis per dataset, but that would be uncommon use case.

src/core/core.controller.js

benmccann · 2020-02-23T15:26:12Z

src/controllers/controller.bar.js

@@ -22,14 +22,14 @@ defaults.set('bar', {
 	},

 	scales: {
-		x: {
+		_index_: {


Why the underscore before and after? I'm assuming you want them to be private, but what's the reason for that? We haven't done this with any other options, so I would probably lean towards just "index"

Because index is not the actual value its going to end up with. Also to try to not collide with user specified values.

benmccann · 2020-02-23T17:56:02Z

src/core/core.controller.js

-				configScales[id],
-				defaultScaleOptions[defaultID]
-			]);
+			helpers.mergeIf(scales[id], [{axis}, configScales[id], defaultScaleOptions[defaultID]]);


I think it might be a bit confusing that there's two ways to do things now. I wonder if we should also replace x and y configs in the other chart types by index and value. E.g. if you wanted to set indexAxis on a bubble or line dataset it wouldn't necessarily just work right now because we only updated the defaults for the BarController. I think it'd be clearer how things worked if we did that universally. And in that case we probably wouldn't want the underscores because I think they're options that users would be setting more frequently

Those controllers use same defaults for both axes, so makes no difference. Its also likely that some (external) chart types want to use some defaults for x and others for y regardless of the indexAxis.

I think you misunderstand how this works. Users are not supposed to use _index_ and _value_ in their configuration. They use 'x' and 'y' (or whatever they like) and the defaults from the _index_ placeholder are applied to the scale that is on the axis marked as indexAxis. And the _value_ defaults are applied to the other scale.

I was thinking it might be nice if they could use index though. Imagine if you wanted to build a chart builder like the dialog to insert a chart in Excel or Google Spreadsheets. You could just toggle back and forth between horizontal and vertical by changing only a single property. Otherwise you need to move all the properties between x and y. I think it'd be fairly messy without using _index_ and _value_ to make a bar chart that can toggle between horizontal and vertical

I see. So we'd require the scales to be defined as index and value. What if the user needs multiple value scales?

Add iScaleID and vScaleID? then the user would be free to name the scales again, but we still need to match the defaults for correct scales.

Yeah, you've raised some good points. Maybe we should just leave it the way it is.

Though I do still wonder if we should use _index_ and _value_ in the other controllers. You said the other controllers use the same defaults for both axes, but I'm not sure that's true. The line controller has a line y-axis and category x-axis, right? Maybe it should be line for _value_ and category for _index_ instead?

benmccann · 2020-02-29T03:22:08Z

docs/charts/bar.md

 | [`borderWidth`](#borderwidth) | <code>number&#124;object</code> | Yes | Yes | `0`
 | [`clip`](#general) | <code>number&#124;object</code> | - | - | `undefined`
 | [`data`](#data-structure) | `object[]` | - | - | **required**
 | [`hoverBackgroundColor`](#interactions) | [`Color`](../general/colors.md) | - | Yes | `undefined`
 | [`hoverBorderColor`](#interactions) | [`Color`](../general/colors.md) | - | Yes | `undefined`
 | [`hoverBorderWidth`](#interactions) | `number` | - | Yes | `1`
+| [`indexAxis`](#general) | `string` | `'x'` | The base axis for the dataset. Use `'y'` for horizontal bar.


I realized this might be a little confusing in the case that you have multiple axes because it kind of sounds like it's asking for an axis ID. Perhaps it should just be a boolean called horizontal?

benmccann · 2020-02-29T03:31:46Z

src/core/core.controller.js

-				configScales[id],
-				defaultScaleOptions[defaultID]
-			]);
+			helpers.mergeIf(scales[id], [{axis}, configScales[id], defaultScaleOptions[defaultID]]);


Yeah, you've raised some good points. Maybe we should just leave it the way it is.

Though I do still wonder if we should use _index_ and _value_ in the other controllers. You said the other controllers use the same defaults for both axes, but I'm not sure that's true. The line controller has a line y-axis and category x-axis, right? Maybe it should be line for _value_ and category for _index_ instead?

kurkle · 2020-02-29T04:57:45Z

right

Replace horizontalBar with baseAxis: 'y'

dcb41a1

kurkle force-pushed the base-axis branch from b9e86d2 to dcb41a1 Compare February 22, 2020 17:39

migration notes

52355da

kurkle marked this pull request as ready for review February 22, 2020 17:56

kurkle added this to the Version 3.0 milestone Feb 22, 2020

kurkle added the type: infrastructure label Feb 22, 2020

etimberg reviewed Feb 22, 2020

View reviewed changes

Remove unneeded merge

693f81e

kurkle changed the title ~~Replace horizontalBar with baseAxis: 'y'~~ Replace horizontalBar with indexAxis: 'y' Feb 23, 2020

kurkle force-pushed the base-axis branch from 5c4be2b to 8ab7a15 Compare February 23, 2020 08:37

Use index instead of base for consistency, chart level option

19f6ed1

kurkle force-pushed the base-axis branch from 8ab7a15 to 19f6ed1 Compare February 23, 2020 08:40

benmccann reviewed Feb 23, 2020

View reviewed changes

benmccann reviewed Feb 29, 2020

View reviewed changes

kurkle closed this Feb 29, 2020

etimberg mentioned this pull request Feb 29, 2020

State of Chart.js & V3.0 #6598

Closed

benmccann mentioned this pull request Mar 29, 2020

Is it possible to flip x & y view in LineChart? #7232

Closed

kurkle deleted the base-axis branch June 12, 2020 05:33

kurkle mentioned this pull request Jun 16, 2020

Replace horizontalBar with indexAxis: 'y' #7514

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace horizontalBar with indexAxis: 'y' #7144

Replace horizontalBar with indexAxis: 'y' #7144

kurkle commented Feb 22, 2020 •

edited

Loading

benmccann commented Feb 22, 2020

etimberg Feb 22, 2020

kurkle Feb 22, 2020

benmccann Feb 23, 2020

kurkle Feb 23, 2020

benmccann Feb 23, 2020

kurkle Feb 23, 2020

benmccann Feb 23, 2020

kurkle Feb 23, 2020 •

edited

Loading

benmccann Feb 28, 2020

kurkle Feb 28, 2020

benmccann Feb 29, 2020 •

edited

Loading

benmccann Feb 29, 2020

benmccann Feb 29, 2020 •

edited

Loading

kurkle commented Feb 29, 2020

Replace horizontalBar with indexAxis: 'y' #7144

Replace horizontalBar with indexAxis: 'y' #7144

Conversation

kurkle commented Feb 22, 2020 • edited Loading

benmccann commented Feb 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kurkle Feb 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benmccann Feb 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benmccann Feb 29, 2020 • edited Loading

Choose a reason for hiding this comment

kurkle commented Feb 29, 2020

kurkle commented Feb 22, 2020 •

edited

Loading

kurkle Feb 23, 2020 •

edited

Loading

benmccann Feb 29, 2020 •

edited

Loading

benmccann Feb 29, 2020 •

edited

Loading