Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

getPixelForTick is confusing and possibly inconsistent #6715

Closed
benmccann opened this issue Nov 8, 2019 · 4 comments · Fixed by #6724
Closed

getPixelForTick is confusing and possibly inconsistent #6715

benmccann opened this issue Nov 8, 2019 · 4 comments · Fixed by #6724

Comments

@benmccann
Copy link
Contributor

It looks to me like sometimes it's doing getPixelForTick and sometimes it's doing something more akin to getPixelForIndex. This gets especially confusing in certain cases like when autoSkip is enabled or you have auto tick generation on the time scale where ticks and indexes don't map 1:1.

@kurkle
Copy link
Member

kurkle commented Nov 10, 2019

I can't seem to find where its used akin to getPixelForIndex?

I'd keep the data separated from scale interface - so scale does not need to know how data is input, parsed or stored internally by the controllers.

If getPixelForIndex is required, it should be placed in controller. But I'd rather not have such a method, it does not make sense if data is object:

data: { Jan: {o: 10, h: 11, c: 9, l: 8}, Feb: ...}

@etimberg
Copy link
Member

I think getPixelForTick acts like getPixelForIndex in the category scale where that's by design

@benmccann
Copy link
Contributor Author

The main reason I'm confused is that when we autoSkip we pass in tick._index, which is the original index before the autoSkip happened (getPixelForGridLine is a wrapper around getPixelForTick):

lineValue = getPixelForGridLine(me, tick._index || i, offsetGridLines);

@etimberg
Copy link
Member

That may be because for the longest time, the auto skip was just not drawing the ticks. Now that we reduce the tick array into the smaller to draw array, I think we should pass the index in that array

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants