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

Finishing removing old major/minor options #7042

Merged
merged 2 commits into from
Feb 5, 2020

Conversation

benmccann
Copy link
Contributor

#6939 removed the concept of different formatting for major and minor ticks and replaced it with scriptable tick options. I found a place where I forgot to remove the old options

ticks.callback already has a test, so I was able to just remove the old tests and didn't have to add or update any

Copy link
Member

@etimberg etimberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this already have migration docs?

@benmccann benmccann force-pushed the scriptable-tick-cleanup branch from 21755b2 to 1904b79 Compare January 31, 2020 00:35
@benmccann
Copy link
Contributor Author

Yeah, it should be in the migration guide already

etimberg
etimberg previously approved these changes Jan 31, 2020
Copy link
Member

@etimberg etimberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Just want to confirm for my own knowledge, that going forward if I want to bold the major ticks on the time scale for instance, I'd use the scriptable options for the font and check tick.major?

Should we add a sample for that?

@benmccann
Copy link
Contributor Author

Yep! That'd be the way to do it. The financial sample does that: https://www.chartjs.org/samples/master/advanced/financial.html

@etimberg
Copy link
Member

Ah, perfect!

@kurkle
Copy link
Member

kurkle commented Jan 31, 2020

Could fix the samples using fontColor etc under major in this same pr? (in samples/scales/time)

@benmccann
Copy link
Contributor Author

Yeah, good idea. I just copied your changes from #7020 for those two files

Copy link
Member

@kurkle kurkle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably make to major a boolean instead of object, since it only accepts enabled property now.

Just a personal note about scriptable options: I find those handy at times, but also cumbersome for really simple tasks (like font styling). IMO it should not be the only option.

src/scales/scale.time.js Show resolved Hide resolved
@kurkle
Copy link
Member

kurkle commented Feb 3, 2020

This one needs a rebase, I think its good to go after that.

@benmccann benmccann force-pushed the scriptable-tick-cleanup branch from 8f7d8c7 to 674536e Compare February 4, 2020 04:13
@benmccann benmccann force-pushed the scriptable-tick-cleanup branch from 674536e to 2bbdac6 Compare February 4, 2020 04:50
@benmccann benmccann force-pushed the scriptable-tick-cleanup branch from 2bbdac6 to b6200a3 Compare February 4, 2020 04:55
@etimberg etimberg merged commit a30f753 into chartjs:master Feb 5, 2020
@etimberg etimberg added this to the Version 3.0 milestone Feb 5, 2020
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.

3 participants