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

Rename INTERVALS.major to INTERVALS.common #4777

Merged
merged 1 commit into from
Sep 29, 2017

Conversation

benmccann
Copy link
Contributor

@benmccann benmccann commented Sep 19, 2017

This PR uses only common units for both minor and major ticks. The Intervals.major variable indicated if a given unit was eligible to be a major unit. The reason we skipped quarters and weeks is that they were uncommon units. It's very strange to plot week 26 2017, week 27 2017, week 28 2017, etc. People are much more used to seeing days or months, so we should choose one of those units. Skipping these two strange units isn't specific to just major ticks

@@ -261,7 +261,7 @@ function determineUnit(minUnit, min, max, capacity) {
interval = INTERVALS[UNITS[i]];
factor = interval.steps ? interval.steps[interval.steps.length - 1] : MAX_INTEGER;

if (Math.ceil((max - min) / (factor * interval.size)) <= capacity) {
if (Math.ceil((max - min) / (factor * interval.size)) <= capacity && INTERVALS[UNITS[i]].common) {
Copy link
Member

Choose a reason for hiding this comment

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

Better to reuse the previous object access and check a boolean before calling Math.ceil:

if (interval.common && Math.ceil((max - m...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good call. i've updated it

@simonbrunel simonbrunel added this to the Version 2.7.1 milestone Sep 27, 2017
@etimberg etimberg merged commit 8a4ac1e into chartjs:master Sep 29, 2017
yofreke pushed a commit to yofreke/Chart.js that referenced this pull request Dec 30, 2017
exwm pushed a commit to exwm/Chart.js that referenced this pull request Apr 30, 2021
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