Replace tooltip item xLabel and yLabel with label and value

[kurkle]

The move of _getIndexScale() and _getValueScale() to DatasetController, enabled usage for tooltips.

This again allows more generalized defaults and removal of overrides in horizontalBar.

[benmccann]

This comment is not directly related to your PR, which I haven't really reviewed yet, but just has me thinking about the subject generally.

For 3.0 (or maybe before if we provided proper backwards compatibility), I wonder if we'd want to flip this concept in the other direction. E.g. for BarChart:

defaults._set('bar', {
	scales: {
		indexAxes: [{ ...  }],
		valueAxes: [{ ... }]
	},
});

	/**
	 * @private
	 */
	_getXScaleId: function() {
		return horizontal ? this.getMeta().valueAxisID : this.getMeta().indexAxisID;
	},

	/**
	 * @private
	 */
	_getYScaleId: function() {
		return horizontal ? this.getMeta().indexAxisID : this.getMeta().valueAxisID;
	}

The issue is that right now we need a horizontal version of every class to change the default scale options. E.g. if we want a horizontal line chart we need to swap the defaults so that x is the line axis and y is the category axis. I don't think you could have a horizontal option that made a horizontal version of any chart because it'd be really complicated to have default scale option values computed based upon the value of the user-supplied horizontal option.

[simonbrunel]

Looks good, but I thought we would be able to remove the special case in the category scale getLabelForIndex()?

[benmccann]

You would need to update the docs too which currently reference xLabel and yLabel: https://github.com/chartjs/Chart.js/blob/master/docs/configuration/tooltip.md

But I think this change breaks backwards compatibility?

[kurkle]

Looks good, but I thought we would be able to remove the special case in the category scale getLabelForIndex()?

Actually no, unless a getValueLabel / getFormattedValue / getValueForTooltip or equivalent is introduced.
I would add a formatValue (or equivalent) scriptable option with that too.

But I think this change breaks backwards compatibility?

I don't think so - xLabel and yLabel are there still too. Only if someone generates TooltipItems for some reason manually, then this default callback would break. But I don't think its a supported scenario.

[kurkle]

Should these rather be label and value instead of indexLabel and valueLabel?

[simonbrunel]

I don't understand why we can't remove the special case from the category scale, which one shouldn't care if it's an index or value scale since now the tooltip calls getLabelForIndex() on the proper index and value scales.

I also think that's a breaking change for user generating custom tooltip items. I think we should fallback to xLabel and yLabel in case custom items doesn't provide indexLabel and valueLabel.

Should these rather be label and value instead of indexLabel and valueLabel?

+1, though that's the formatted value.

[kurkle]

I don't understand why we can't remove the special case from the category scale, which one shouldn't care if it's an index or value scale since now the tooltip calls getLabelForIndex() on the proper index and value scales.

Because if we have 2 labels and 100 data points, we need to know the label index for (for example) data point 20, and we actually have only the label as data, not its index.
Eg. labels: [A,B], data: [A,B,B,A]
We have indexes 0-3 and values A/B. Scale has (label) indexes 0-1 with values A and B.
So for value axis we are asking for the value actually, not the label (and I agree its not the right function we are using to get it).

I also think that's a breaking change for user generating custom tooltip items. I think we should fallback to xLabel and yLabel in case custom items doesn't provide indexLabel and valueLabel.

I initially put it that way, but thought it might be useless and left those fallbacks out (its a lot prettier without. 😄 ).

+1, though that's the formatted value.

At the moment it actually is not the formatted value (except for time scale). That should be generalized too.

[benmccann]

+1 to label and value instead of indexLabel and valueLabel

[kurkle]

I don't understand why we can't remove the special case from the category scale, which one shouldn't care if it's an index or value scale since now the tooltip calls getLabelForIndex() on the proper index and value scales.

Time scale implements same thing. I think other scales should do that as well (probably why bar does not work on linear x-axis)

I think this aspect needs more work, but its not in the scope of this PR.

I also think that's a breaking change for user generating custom tooltip items. I think we should fallback to xLabel and yLabel in case custom items doesn't provide indexLabel and valueLabel.

Original functionality restored as fallback.

As a conversation starter added a format option to scale, that should be a function and is used to format the value for tooltip (why not for ticks too).

[simonbrunel]

As a conversation starter added a format option to scale

I think that's a great idea but should be implemented as a separate PR.

What about formatter instead? which is less confusing IMO but also consistent with the datalabels option. I would design this feature as a non indexable / non scriptable option but as a contextual function (as in the datalabels plugin) and with the following signature: formatter(any value, object context)

I would also move this option under the tooltip option in a first time, then add another one for the scale in a second time (or vice-versa).

[kurkle]

Conversation started, so removed "value formatting" from this PR. Will do another about that.

[benmccann]

Don't forget to update ./docs/configuration/tooltip.md and ./docs/axes/cartesian/category.md to mention the new index and value options (probably in place of where xValue and yValue are used today)

[kurkle]

Added minimal documentation. I don't think anything needs to be changed in ./docs/axes/cartesian/category.md

[benmccann]

You're right. Nevermind about category.md

[benmccann]

@kurkle just a heads up, this PE will need to be rebased