Fixed tooltip labelling on Bar Chart when min is defined (#3618) #3620
Conversation
Jareechang
force-pushed
the
issue/#3618
branch
from
b0b3293
to
e213e62
Compare
| var min = chartConfig.options.scales.xAxes[0].ticks.min; | ||
| var labels = chartConfig.data.labels; | ||
|
|
||
| if (!element._adjustedIndex && min) { |
There was a problem hiding this comment.
Is this condition min !== 0 or min !== undefined?
There was a problem hiding this comment.
the condition should check min !== undefined. I may need to rethink this adjustment because this "adjustment" in the index is very specific to the Barchart.
| element._index -= labels.indexOf(min); | ||
| element._adjustedIndex = true; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Could this method directly be on the element class instead?
adjustIndex: function(config) {
var min = config.options.scales.xAxes[0].ticks.min;
if (this._adjustedIndex || min === undefined) {
return;
}
this._index -= config.data.labels.indexOf(min);
this._adjustedIndex = true;
}There was a problem hiding this comment.
@simonbrunel, yes, this is a better place for the helper. I'll go ahead and update that
| @@ -21,6 +22,7 @@ module.exports = function(Chart) { | |||
| for (j = 0, jlen = meta.data.length; j < jlen; ++j) { | |||
| var element = meta.data[j]; | |||
| if (!element._view.skip) { | |||
| helpers.adjustIndex(element, chartConfig); | |||
There was a problem hiding this comment.
chartConfig is only called from here, chart.config should be used instead:
element.adjustIndex(chart.config);There was a problem hiding this comment.
good call, I will make this edit
| @@ -53,6 +53,16 @@ module.exports = function(Chart) { | |||
| } | |||
| return base; | |||
| }; | |||
| // Adjust element index when min is defined | |||
| helpers.adjustIndex = function(element, chartConfig) { | |||
| var min = chartConfig.options.scales.xAxes[0].ticks.min; | |||
There was a problem hiding this comment.
What if the element doesn't belong to the first axis (xAxes[0])?
There was a problem hiding this comment.
hmm I was looking into scatter plot where you can define multiple xAxes with different ticks.min... which is the case I think you are referring to.
as per documentation:
| Name | Type | Default | Description |
|---|---|---|---|
| scales.xAxes | Array | The bar chart officially supports only 1 x-axis but uses an array to keep the API consistent. Use a scatter chart if you need multiple x axes. |
how about if I make this patch specific to when only one xAxes is defined (Given scatter plot is only time where multi xAxes is possible) ?
Jareechang
force-pushed
the
issue/#3618
branch
from
e213e62
to
f6ce4a4
Compare
added helper method to adjust the index pass in chartConfig rather than access within method, make it easier to test added semi-colon at the end of helper method added test for adjustIndex helper method fixed lint issues added integration test for the interaction of trigger an event over the bar . . moved adjustIndex into element helper removed method from helper and adjusted method in core.interaction added test for the element adjustIndex helper added a skipIndexAdjustment method to handle when to skip the adjustment along with test cases fixed lint issues removed the test for the helper method
Jareechang
force-pushed
the
issue/#3618
branch
from
f6ce4a4
to
3df130a
Compare
|
hi @simonbrunel, i've made the requested changes. Could you please let me know how it looks 😄 thanks! |
As title states, create a fix for #3618.
summary of changes: