-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
add shouldResetTickmarkLabels #1591
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).
@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 |
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
src/api/create-chart.ts
Outdated
* Use this if you want to extend the default behavior of the horizontal time scale. | ||
* This should be used with the createChartEx function | ||
* | ||
* @returns An uninitialized class implementing the {@link IHorzScaleBehavior}`<Time>` interface |
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.
We could probably 'link' Time
as well here.
src/api/create-chart.ts
Outdated
* Use this if you want to extend the default behavior of the horizontal time scale. | ||
* This should be used with the createChartEx function |
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.
This was 'placeholder' text. Lets use a more detailed and helpful description.
/**
* Provides the default implementation of the horizontal scale (time-based) that can be used as a base for extending the horizontal scale with custom behavior.
* This allows for the introduction of custom functionality without re-implementing the entire {@link IHorzScaleBehavior<Time>} interface.
*
* For further details, refer to the {@link createChartEx} chart constructor method.
*
* @returns {new () => IHorzScaleBehavior<Time>} - An uninitialized class implementing the {@link IHorzScaleBehavior<Time>} interface.
*/
Type of PR: bugfix
PR checklist:
Overview of change:
Added the
shouldResetTickmarkLabels
method toIHorzScaleBehavior
and e2e test with an example of how users could fix issues like #1588 using this method