Skip to content

Make all ticks objects#6645

Merged
etimberg merged 2 commits intochartjs:masterfrom
benmccann:ticks-cleanup
Oct 31, 2019
Merged

Make all ticks objects#6645
etimberg merged 2 commits intochartjs:masterfrom
benmccann:ticks-cleanup

Conversation

@benmccann
Copy link
Contributor

@benmccann benmccann commented Oct 30, 2019

Closes #5019

There's a ton of tests that will need to be updated to expect tick objects instead of strings, but otherwise this is mostly working. I wanted to get some feedback before I finish that

A couple things that could be cleaned up:

  • There's a few places that could be less code if we could use es6, but the linter won't let us yet. Allow using es6 syntax for Chart.js 3.x eslint-config-chartjs#1
  • convertTicksToLabels would probably be less code if it modified ticks in place to set the labels rather than returning the labels and then having core.scale put the labels on the ticks

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.

Couple of suggestions, but I'm good with this implementation too.
This is a good cleanup!

@etimberg
Copy link
Member

I like the direction this is going. I agree with @kurkle's points above. Updating to generateTickLabels will also make the interface clearer I think

@kurkle
Copy link
Member

kurkle commented Oct 30, 2019

I think you can go ahead and fix the tests.

@benmccann benmccann force-pushed the ticks-cleanup branch 2 times, most recently from 80e308c to fe29898 Compare October 30, 2019 21:17
@benmccann
Copy link
Contributor Author

Ok. All tests should be passing now

etimberg
etimberg previously approved these changes Oct 30, 2019
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.

I didn't have any complaints.

kurkle
kurkle previously approved these changes Oct 31, 2019
etimberg
etimberg previously approved these changes Oct 31, 2019
@benmccann benmccann dismissed stale reviews from etimberg and kurkle via 1b729a5 October 31, 2019 15:31
@etimberg
Copy link
Member

shall we merge this?

@benmccann
Copy link
Contributor Author

I'm ready if you both are

@etimberg etimberg merged commit 9981132 into chartjs:master Oct 31, 2019
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.

Consider making ticks objects

3 participants