Skip to content
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

Fix time series scale to have each data point is spread equidistant #11388

Merged
merged 3 commits into from
Jul 13, 2023

Conversation

stockiNail
Copy link
Contributor

@stockiNail stockiNail commented Jul 4, 2023

Fix #11371

Master PR
time-series old time-series new

TODO

  • test cases to update and add

Copy link
Member

@etimberg etimberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall I think this makes sense and seems to more closely match the old behaviour. @LeeLenaleee @kurkle do you recall how the original v3 logic came to be?

@LeeLenaleee
Copy link
Collaborator

No, don't know. That happend before I got involved

@kurkle
Copy link
Member

kurkle commented Jul 5, 2023

Overall I think this makes sense and seems to more closely match the old behaviour. @LeeLenaleee @kurkle do you recall how the original v3 logic came to be?

I do not recall.

I did not dig to see if this new behaviour can happen with ticks.source = 'labels'? IMO it should only be done if source is 'data'.

@stockiNail
Copy link
Contributor Author

I did not dig to see if this new behaviour can happen with ticks.source = 'labels'? IMO it should only be done if source is 'data'.

@kurkle the timeseries scale is extending the time scale. THis PR impl overrides the _generate method. And this method is invoked ONLY when the tick.source is NOT 'labels'.
If labels, the time scale is already managing using the labels as ticks, therefore nothing to do there.

See time scale source:

  buildTicks() {
    const options = this.options;
    const timeOpts = options.time;
    const tickOpts = options.ticks;
    const timestamps = tickOpts.source === 'labels' ? this.getLabelTimestamps() : this._generate();

@stockiNail stockiNail marked this pull request as ready for review July 6, 2023 13:26
@stockiNail stockiNail requested review from kurkle and etimberg July 6, 2023 15:33
@stockiNail
Copy link
Contributor Author

@etimberg @kurkle @LeeLenaleee I think I should be done.
If I may, just a thing. In the doc is written:

The time series scale extends from the time scale and supports all the same options. However, for the time series scale, each data point is spread equidistant.

This is partially true. Timeseries scale does not support all options, for instance stepSize. This was so in v2 as well.

@etimberg
Copy link
Member

etimberg commented Jul 7, 2023

@etimberg @kurkle @LeeLenaleee I think I should be done. If I may, just a thing. In the doc is written:

The time series scale extends from the time scale and supports all the same options. However, for the time series scale, each data point is spread equidistant.

This is partially true. Timeseries scale does not support all options, for instance stepSize. This was so in v2 as well.

Lets leave the docs for another PR, as that will be rather slow to audit each setting

@LeeLenaleee LeeLenaleee added this to the Version 4.3.1 milestone Jul 13, 2023
@etimberg etimberg merged commit 05608b0 into chartjs:master Jul 13, 2023
@stockiNail stockiNail deleted the timeSeries branch July 14, 2023 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

v2 to v4 timeseries migration
4 participants