replace day mark with month if month didn't fit #1591
Conversation
There was a problem hiding this comment.
The changes work well on the test case, however I'm concerned about implementing this fix within the tick-marks.ts file. This fix is specific to the case where the horizontal scale is time based. So I think it would be better to try fix this within the horz-scale-behavior-time.ts and time-scale-point-weight-generator.ts files since that code is only used for a time based scale and it is only run when new data is provided instead of when the chart is scrolled or zoomed (therefore the performance should be better as well).
src/model/tick-marks.ts
Outdated
| const startOfTheMonth = new Date(mark.originalTime * 1000); | ||
| startOfTheMonth.setDate(1); | ||
| startOfTheMonth.setHours(0); | ||
| startOfTheMonth.setMinutes(0); | ||
| startOfTheMonth.setSeconds(0); | ||
| const startOfTheMonthUtcSeconds = Math.floor(startOfTheMonth.getTime() / 1000); |
There was a problem hiding this comment.
Could you do some benchmarking and determine the cost of getting the start of the month timestamp? Since for a given timestamp the calculated 'start of month' timestamp should always be the same, this means we could memo the value for a bit more performance. However lets only do this if it is quicker.
There was a problem hiding this comment.
it will make sense if there are lot of day marks, but we can have a 30 at max, and we still need to build a cache key from the timestamp for each item to check if the value in cache, so there is no profit of implementing memo
@SlicedSilver I addressed all other issues but for this one. it probably make more sense to leave it here. due to all marks are valid and the issue dependent on zoom and/or width of the chart and this check need to be triggered on each buildTickMarks not only on data change and we can't fix it in |
| startOfTheMonth.setHours(0, 0, 0, 0); | ||
|
|
||
| const startOfTheMonthUtcSeconds = Math.floor(startOfTheMonth.getTime() / 1000); |
There was a problem hiding this comment.
If we use .setHours(0, 0, 0, 0) then the time should be even multiple of 1000 so we wouldn't need to use floor here. However, perhaps this will catch some edge case I'm not aware of so it is probably safer to keep it in.
| import { fixMonthMarks } from './horz-scale-behavior-time/time-utils'; | ||
| import { TickMarkWeight } from './horz-scale-behavior-time/types'; |
There was a problem hiding this comment.
I still feel that this file (tick-marks.ts) shouldn't 'know' about the type of horizontal scale. The behaviour should apply to all types of scales OR the logic for handling this issue should be within the horz-scale-behavior-time code.
As an example, if I implemented a custom horizontal scale and happened to use 50 and 60 as tick weights then this behaviour would be triggered even though my weights have nothing to do with months and days.
|
hi @eugene-korobko, could you pls check this PR and the issue? what do you think about this approach, is it okay? as an alternative instead of changing day tickmark to month we could hide the tickmark entirely |
| const startOfTheMonthUtcSeconds = Math.floor(startOfTheMonth.getTime() / 1000); | ||
|
|
||
| const monthMark = marks.find((m: TickMark) => { | ||
| return m.weight === TickMarkWeight.Month && (m.originalTime as number) >= startOfTheMonthUtcSeconds; |
There was a problem hiding this comment.
is m.originalTime as number conversion correct? Several lines above we call convertTime(mark.originalTime as Time)
lets check this code with different format of input data
There was a problem hiding this comment.
Thanks for catching that, added convertTime
Type of PR: bugfix
PR checklist:
Overview of change:
In some cases the month mark can't be placed on the timescale, but some day marks of the current month fit and we have strange behavior when we have on timescale
Jan Feb 21 Apr. as a fix I've added a check for this specific case and change the weight of the mark before placing it into timescale