Skip to content

Option for determining if tick is major#6260

Closed
benmccann wants to merge 1 commit intochartjs:masterfrom
benmccann:isMajor
Closed

Option for determining if tick is major#6260
benmccann wants to merge 1 commit intochartjs:masterfrom
benmccann:isMajor

Conversation

@benmccann
Copy link
Contributor

@benmccann benmccann commented May 9, 2019

The current code has hardcoded that a major tick is one that aligns with startOf(value, majorUnit). However, I've found many cases where I want a different behavior. E.g. in the financial charts we don't have weekends, so we might want to say the major tick is the first weekday of the month so that we have a majorTick begin each month. This adds an option called isMajor which takes a function to make the logic configurable.

@kurkle
Copy link
Member

kurkle commented May 9, 2019

I think you should be able to change major in afterBuildTicks?

@benmccann
Copy link
Contributor Author

I think you should be able to change major in afterBuildTicks?

As a user, it's much harder to create an entirely new scale type overriding afterBuildTicks than it is to specify an option on an existing scale type. I'd also have to access numerous private variables in order to accomplish that in afterBuildTicks

@kurkle
Copy link
Member

kurkle commented May 9, 2019

I think you should be able to change major in afterBuildTicks?

As a user, it's much harder to create an entirely new scale type overriding afterBuildTicks than it is to specify an option on an existing scale type. I'd also have to access numerous private variables in order to accomplish that in afterBuildTicks

I was referring to the callback: https://www.chartjs.org/docs/latest/axes/#callbacks

@benmccann
Copy link
Contributor Author

benmccann commented May 9, 2019

Ah, yeah, that would probably work if we made _adapter and _unit / _majorUnit public or added them as callback arguments. We'd also need the fix this is making in getLabelForIndex

@benmccann
Copy link
Contributor Author

I think this is actually required after all to avoid major performance regression in #6274. I shared an explanation in Slack. Maybe easier to discuss there to cover the different cases

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants