Improved code minimization#6362
Conversation
kurkle
left a comment
There was a problem hiding this comment.
Looks good, but not sure if _getLabels is right function to use.
| } | ||
|
|
||
| return me.ticks[index - me.minIndex]; | ||
| return me._getLabels()[index]; |
There was a problem hiding this comment.
Is altering of ticks in ticks.callback or afterBuildTicks supported?
me.getTicks()[index].label instead, in that case?
There was a problem hiding this comment.
I believe it wouldn't be supported because there's no other scale that accesses ticks in getLabelForIndex and this method is only used by the tooltip which a separate concept from ticks. The way we've always suggested customizing the tooltip is via https://www.chartjs.org/docs/latest/configuration/tooltip.html?q=#external-custom-tooltips
|
I've rebased this PR |
|
@kurkle did my response address your concern or do you think I should change it back? |
kurkle
left a comment
There was a problem hiding this comment.
I think we should not assume that category scale is fed with only plain array data. So label index might not be same as data index. Anyway, its not what this PR is trying to do, so I think its fine for now.
|
@benmccann @etimberg don't forget to rewrite the commit message when merging ("Minor cleanup" isn't helpful), but also add a label and milestone to every merged PR, it really helps to write release notes :) |
|
Ok. I've renamed to "Improved code minimization" which hopefully helps describe it a bit better |
|
Thanks! we should make sure to rewrite the commit message (at least the commit title - first line) before merging so the git history (and blaming) contains more interesting information. |
An assortment of random improvements I came across in the past couple days