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

[BUG] Use time.unit option to create default min/max for empty chart #5045

Merged

Conversation

jcopperfield
Copy link
Contributor

Currently the default min/max values for the time scale are create with the day unit.
If the user has set the time.unit option and has hidden all the datasets, the scale will be empty (no tick labels).
Using the time.unit option will show the labels as selected.
pr-empty-time-chart-default-scale-unit

@simonbrunel
Copy link
Member

Looks good, I would add unit tests next to this one and this one that check usage of time.unit when explicitly set.

etimberg
etimberg previously approved these changes Dec 15, 2017
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.

I agree with @simonbrunel about a test. Other than that, looks great!

@jcopperfield
Copy link
Contributor Author

@simonbrunel I don't know what you are expecting the code to do in those 2 test situations. The PR sets the scale limits for auto generated timestamps. When ticks.source is labels or data the result would still be an empty, due to the code in buildTicks.

@simonbrunel
Copy link
Member

It's not specific to auto generated timestamps but to all cases where there is no labels or data or explicit min/max to compute a valid data range. If ticks.source == 'labels' | 'data' and if there is no labels or data, the previous behavior was to generate a min/max range for a whole day. Your PR changes that behavior when there is an explicit time.unit so we should have this kind of tests:

it ('should correctly handle empty `data.labels` using `time.unit`', function() {
    var chart = this.chart;
    var scale = chart.scales.x;
    var options = chart.options.scales.xAxes[0];

    options.time.unit = 'year';
    chart.data.labels = [];
    chart.update();

    expect(scale.min).toEqual(+moment().startOf('year'));
    expect(scale.max).toEqual(+moment().endOf('year') + 1);
    expect(getTicksLabels(scale)).toEqual([]);
});

We should also rename tests I linked for something more explicit, like:

it ('should correctly handle empty `data.labels` using "day" if `time.unit` is undefined', ...

Copy link
Member

@simonbrunel simonbrunel left a comment

Choose a reason for hiding this comment

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

Thanks @jcopperfield

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.

None yet

3 participants