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

Parse labels once when building time scale #3826

Merged
merged 1 commit into from
Feb 10, 2017

Conversation

tredston
Copy link
Contributor

The input labels&data is parsed and converted into moments in determineDataLimits; reuse these moments in buildLabelDiffs instead of duplicating the work.

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 think this looks fine. Can you squash it to a single commit?

The input labels/data is converted into moments in `determineDataLimits`, reuse them instead of duplicating the work.
@tredston
Copy link
Contributor Author

No problem, squashed.

@@ -485,68 +485,4 @@ describe('Time scale tests', function() {
threshold: 0.75
});
});

it('should not throw an error if the datasetIndex is out of bounds', function() {
Copy link
Member

Choose a reason for hiding this comment

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

Why don't we need these tests anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The method being tested (getLabelMoment) was unused and has been removed.

Copy link
Member

Choose a reason for hiding this comment

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

I have read only the test title and didn't notice it was explicitly testing this method.

@etimberg etimberg added this to the Version 2.6 milestone Jan 28, 2017
@etimberg etimberg merged commit 9f3b51a into chartjs:master Feb 10, 2017
@tredston tredston deleted the feature/parse-once branch February 15, 2017 00:05
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