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
replace day mark with month if month didn't fit #1591
base: master
Are you sure you want to change the base?
Conversation
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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