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

Add back Chart.Ticks.formatters #5088

Merged
merged 1 commit into from
Jan 6, 2018
Merged

Conversation

benmccann
Copy link
Contributor

@benmccann benmccann commented Dec 28, 2017

Addresses #5021

Seems like we were too agressive. There's a few different people (me, @adamk33n3r, @cholt0425) that want to be able to use the default tick formatters that were previously available via Ticks.formatters. Accordingly, this PR adds back Ticks.formatters, but not Ticks.generators, which is what I think we were most interested in making private

@benmccann benmccann changed the title Add back Chart.Ticks Add back Chart.Ticks.formatters Dec 28, 2017
etimberg
etimberg previously approved these changes Dec 28, 2017
@simonbrunel simonbrunel added this to the Version 2.8 milestone Jan 6, 2018
simonbrunel
simonbrunel previously approved these changes Jan 6, 2018
@simonbrunel
Copy link
Member

Looks good, I would add unit tests (core.ticks.tests.js) to make sure that we don't remove Chart.Ticks.* methods:

expect(typeof Chart.Ticks).toBeDefined()
expect(typeof Chart.Ticks.formatters).toBeDefined()
expect(typeof Chart.Ticks.formatters.value).toBe('function')
// ...

@benmccann
Copy link
Contributor Author

Thanks for taking a look. I've added the tests

it('Should expose formatters api', function() {
expect(typeof Chart.Ticks).toBeDefined();
expect(typeof Chart.Ticks.formatters).toBeDefined();
expect(typeof Chart.Ticks.formatters.linear).toBe('function');
Copy link
Member

Choose a reason for hiding this comment

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

I think it's safer to add all:

expect(typeof Chart.Ticks.formatters.linear).toBe('function');
expect(typeof Chart.Ticks.formatters.logarithmic).toBe('function');
expect(typeof Chart.Ticks.formatters.value).toBe('function');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. Updated

Copy link
Member

@simonbrunel simonbrunel left a comment

Choose a reason for hiding this comment

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

Thanks @benmccann

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.

3 participants