Fix duplicate labels on the TimeSeriesChart x-axis #443
Conversation
| "month": [...Array(getDaysInMonth(new Date()) + 1).keys()].map(index => subDays(new Date(), index)).reverse(), | ||
| "week": [...Array(8).keys()].map(index => subDays(new Date(), index)).reverse(), | ||
| "day": [...Array(25).keys()].map(index => subHours(new Date(), index)).reverse(), | ||
| "hour": [...Array(61).keys()].map(index => subMinutes(new Date(), index)).reverse(), |
There was a problem hiding this comment.
Are these numbers arbitrary or where do they come from?
There was a problem hiding this comment.
Maybe I should have explained this.
To prevent duplicate labels I set the ticks manually.
The hour chart should have a maximum of 61 ticks, meaning 1 for every minute in an hour plus one more so the first and last ticks have the same time.
Likewise, the day chart should have 25 ticks since there are 24 hours in a day + 1 hour for the first tick.
It works analogously for the week and the month chart.
| @@ -36,8 +36,33 @@ class TimeSeriesChart extends React.Component { | |||
| resolution, | |||
| } = this.props; | |||
|
|
|||
| console.log(data); | |||
sindresorhus
changed the title
|
Thanks for working on this, @benv2 😃👍 |
|
Make sure you run |
Set domain to auto as manual setting was causing errors. Removed code console.log mentioned in code review.
|
I fixed the linting violations and improved the code a bit. |
| month: [...new Array(getDaysInMonth(new Date()) + 1).keys()].map(index => subDays(new Date(), index)).reverse(), | ||
| week: [...new Array(8).keys()].map(index => subDays(new Date(), index)).reverse(), | ||
| day: [...new Array(25).keys()].map(index => subHours(new Date(), index)).reverse(), | ||
| hour: [...new Array(61).keys()].map(index => subMinutes(new Date(), index)).reverse(), |
There was a problem hiding this comment.
Nitpick, but it's wasteful to create so many date objects. I would define const now = new Date(); above const ticks and use that.
You could also abstract some of the logic into a helper function:
const makeTicks = (count, fn) => {
return [...new Array(count).keys()].map(index => fn(new Date(), index)).reverse();
};| @@ -38,6 +38,17 @@ class TimeSeriesChart extends React.Component { | |||
|
|
|||
| const labelFormat = resolutionToLabelFormat.get(resolution); | |||
|
|
|||
| const getTicks = resolution => { | |||
There was a problem hiding this comment.
You can move this to the top-level, below the resolutionToLabelFormat function, since it doesn't depend on any internal state.
|
As you suggested I changed the multiple new dates to a constant. This makes this possible: const getTicks = resolution => {
const ticks = {
year: makeTicks(13, 'month'),
month: makeTicks(getDaysInMonth(now) + 1, 'day'),
week: makeTicks(8, 'day'),
day: makeTicks(25, 'hour'),
hour: makeTicks(61, 'minute'),
};
...I hope this makes it easier to understand what the code does. |
| day: (date, index) => subDays(date, index), | ||
| hour: (date, index) => subHours(date, index), | ||
| minute: (date, index) => subMinutes(date, index), | ||
| }; |
There was a problem hiding this comment.
I don't really see the point of this. It just makes it more verbose. makeTicks(13, 'month') is not much clearer than makeTicks(13, subMonths).
There was a problem hiding this comment.
My bad. I forgot I could write it like that.
I passed (date, index) => subDays(date, index) instead of subDays which made it look relatively messy.
Will fix this!
| @@ -25,6 +25,29 @@ const resolutionToLabelFormat = new Map([ | |||
| ['all', 'MMMM YYYY'], | |||
| ]); | |||
|
|
|||
| const now = new Date(); | |||
There was a problem hiding this comment.
This needs to be inside the makeTicks function as we need the date when getTicks called, not when the module is evaluated.
|
I improved the code with your suggestions. 👍 I noticed though that sometimes the chart has more data than necessary. This results in the first tick being offset. This is especially noticeable on the day chart.
|
|
This is looking great! We really appreciate the help, @benv2 🙌 |
Weird. I can't reproduce that. Maybe it depends on when in the day you check. If you still see this, would you be interested in looking into it? |
|
Thank you for the guidance @sindresorhus .
I'll try to reproduce it and see what I can do. This is a bit off-topic but I was wondering if you still need help translating the app. |
Definitely, that would be great! We still have some untranslated strings in German and we expect more in the future. I've sent you an invite to our Crowdin project (It's what we use to translate the app). Feel free to look through the existing translated strings too and see if you find any mistakes or improvements ;) |
|
@sindresorhus Cool. Will do! 👍 |
Did my best to fix #311
I'm pretty new to open source so I'm not entirely sure if I'm doing this right.