-
-
Notifications
You must be signed in to change notification settings - Fork 968
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
Change: Improve graph period markings #8732
Conversation
I didn't center labels on their column because they skip three months at a time. Left-align labels seem to denote the left grid line as the start of that three-month data period without confusing the user who might think the data point is only January data, for example. I tried the Roman numerals suggested in #8690 but it was a bit messy. Financial quarter markings (Q1, etc) would have been nice but fiscal years IRL don't often match calendar years and differ by country. Centering years might look odd when you're partway through a year, and would have to center on however many periods exist. I'm not opposed to eliminating vertical grid lines, but right now my skills support iterative improvement much better than reinventing the wheel. 😀 I'd welcome some other opinions before I go changing long-established things left and right. |
I meant centering labels on the grid line, not on the column/data point itself. Having the point itself in the middle to mean the period is fine. Highcharts image I posted is a monthly chart so ofc I don't suggest copying it completely. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really appreciate you thought this through; the additional information you give in your comment would be nice to have in the motivation next time, but creating PRs is also an iterative process :D
Personally:
- The lighter lines per year are a great addition, much more readable.
- Removing "Mar", "Jun", "Sep" and "Dec" does not yield any loss of data, so I really like that too. Less is more :)
So +1 for me! (and of course, @ldpl has some nice suggestions to further improve it, but I am so-so if that should be done in this PR or in a later; for me, that is up to you :))
3a7a5ef
to
8b3449b
Compare
Ah, I misunderstood your centering suggestion. Centered labels would mean the very first month (Apr) would collide with the text for $0. Even if that were solved, I am not a fan of centered text in this case, but won't argue against it if you or someone else makes a PR to change it in the future. @TrueBrain Separate PRs are good, I think. Ready for review. 😄 |
Tried to implement what is suggested: https://pastebin.com/DnK9QPij (also removes vert lines in cargo payment graph but I think it's fine) Also noticed a bug that if the font is large enough year gets clipped (it's been there before as well). |
Personally, if you go this route, you should also move the points to the left. Now you see Edit: correction, my issue is not with the location of the dot, but with the fact it is a dot while it is a value of the whole quarter. But that is a completely different can of worms :P |
I like the centered years, but am still not a fan of centered months. Edit: I don't disagree with TB that the points could be moved to the vertical grid lines (which are no longer drawn) but then it almost looks like the data points are cherry-picked from January, April, July, and October, with the months in between discarded, rather than taking the average of each three-month period. To keep vertical lines in the cargo payment graph, just put the vertical line code inside |
8b3449b
to
17d5c40
Compare
17d5c40
to
74d1808
Compare
I did some experimentation with @ldpl's suggestions and ran into some design issues. I'd like to move forward as-is and leave those for a future PR.
I did fix one more colour constant and included my changes in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Are there eints considerations regarding the translations?
I checked with boss-man, there is not. |
Motivation / Problem
Periods of graphs are hard to read, due to lots of small text.
Description
This PR simplifies the text and adds brighter grid lines separating each year.
Closes #8632.
There is also some code cleanup of colour definitions which I missed in #8690.
Limitations
Checklist for review
Some things are not automated, and forgotten often. This list is a reminder for the reviewers.