Fix #38: Trend diagram: Correct x-axis decade labels for decades 1900 and before #176
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
results.js: view.GraphResults.renderGraph: - Render correct x-axis decade labels in the trend diagram for decades 1900 and before by passing to Rickshaw.Graph.Axis.Time constructor the existing fixed time.ceil function with special handling of decades. (Previously, labels for decades 1900 and before were shifted back by one (1890 instead of 1900 and so on), probably resulting from Rickshaw's approximate handling of leap years.) - Modify the fixed time.ceil to work for decades 1800 and before.
|
Tested and merged! Really great to get this fixed. Sorry it took so long. |
|
Great; thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes
Fix issue #38:
time.ceil(Rickshaw.Fixtures.Time.ceil) function with special handling of decades.time.ceilto work correctly for decades 1800 and before.Previous, incorrect behaviour
Previously, labels for decades 1900 and before were incorrectly shifted back by one (1890 instead of 1900 and so on), probably resulting from Rickshaw's approximate handling of leap years that assumes that each year has 365.25 days. (The years 1900, 1800 and 1700 were not leap years.)
Tests
I have tested the change only interactively on a Korp 9 test instance of the Language Bank of Finland. As far as I can see, the labels are now correct:
However, as the fixed
time.ceilhad been there since commit b917cc39 but not used, I’m wondering if your reason not to use it might also apply to this fix, too. I haven’t noticed any unwanted side-effects, though.Other comments
The comments and commented-out code around the change might not be relevant any longer, but I only removed (or moved) those that I was fairly convinced of. If you find my comments in the code too verbose, unnecessary or deficient, please tell me.
The commit message is rather different in style from your commit messages, so if you find it too verbose or pedantic, please tell me.
And if you’d prefer a different kind of a pull request description, please also tell me that, as I hope this is not the last pull request I’ll be creating for Korp.