Skip to content

Conversation

etimberg
Copy link
Member

Reverts #3620 per comments in #3618
@KoyoSE would this fix the issue you found?

@KoyoSE
Copy link
Contributor

KoyoSE commented Nov 29, 2016

@etimberg Yes. I think this fix does not solve the fundamental problem.

@etimberg
Copy link
Member Author

@KoyoSE what's the correct way to solve the underlying problem?

@KoyoSE
Copy link
Contributor

KoyoSE commented Nov 29, 2016

@etimberg Good morning. Ah... just revents #3620. I think #3649 is correct way (better way). If it merge both, the corrected tooltip will shift again. :)

I think underlying problem is here. (already fixed by #3649.)
https://github.com/KoyoSE/Chart.js/blob/98670d2f5f93ff9f1612a39820730ffe69b68af2/src/scales/scale.category.js#L57-L60

@Jareechang I am sorry about your efforts.

@Jareechang
Copy link
Contributor

hi @KoyoSE, your change is essentially the same as mine. Our code basically do the same thing.

I've written unit test to cover the issue. The only thing I can see why it won't work is because we're both apply same changes.

Either way, I do like how simple your code is. However, getLabelForIndex method is what the name suggest — getting the label. It shouldn't do anything else.

Hence, i've add adjustIndex method in core.element.js.

@KoyoSE
Copy link
Contributor

KoyoSE commented Nov 30, 2016

Hello @Jareechang

However, getLabelForIndex method is what the name suggest — getting the label. It shouldn't do anything else.

I understand what you say.
...
( I think I'm something wrong. Now check again. Just moment.)

@KoyoSE
Copy link
Contributor

KoyoSE commented Dec 1, 2016

List-1 is the first code.(code of before)

list-1

getLabelForIndex: function(index, datasetIndex) {
		var me = this;
		var data = me.chart.data;
		var isHorizontal = me.isHorizontal();

		if ((data.xLabels && isHorizontal) || (data.yLabels && !isHorizontal)) {
			return me.getRightValue(data.datasets[datasetIndex].data[index]);
		}

		return me.ticks[index];  
	},

ex)
labels = 1,2,3,4,5,6
min = 2
ticks = 2,3,4,5,6
Index is from 0.
Therefore, the label shifts. and wrong label returned .

List-2 is the code I changed. = current code

list-2

getLabelForIndex: function(index, datasetIndex) {
		var me = this;
		var data = me.chart.data;
		var isHorizontal = me.isHorizontal();

		if (data.yLabels && !isHorizontal) { // <<< for xLabels yLabels bug
			return me.getRightValue(data.datasets[datasetIndex].data[index]);
		}

		return me.ticks[index - me.minIndex];  // <<<
	},

ticks is from min then I added " - me.minIndex".

Actually, should we do like this(list-3)?

list-3

getLabelForIndex: function(index, datasetIndex) {
		var me = this;
		var data = me.chart.data;
		var isHorizontal = me.isHorizontal();
		var labels = me.getLabels();  // <<<

		if (data.yLabels && !isHorizontal) {
			return me.getRightValue(data.datasets[datasetIndex].data[index]);
		}

		return labels[index]; // <<< 
	},

I think it is a simple bug.
It is labels, not ticks.
Bugs should fix for this function.

list-2 & list-3 is the same result, but List-3 may be better.

By the way, #3620 error occurs in the horizontalBar , Radar-chart.
If merge #3620, we need to fix it and revert #3649.

current code is here :
https://github.com/chartjs/Chart.js/blob/master/src/scales/scale.category.js#L52-L61

@etimberg etimberg merged commit 3ff58e5 into master Dec 1, 2016
@KoyoSE KoyoSE mentioned this pull request Dec 2, 2016
@etimberg etimberg deleted the revert-3620-issue/#3618 branch December 4, 2016 00:56
@simonbrunel simonbrunel added this to the Version 2.5 milestone Jan 29, 2017
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.

4 participants