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.

Sign up for GitHub

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

Jump to bottom

fix: old x axis labels were not destroyed during data update if parsing was set to false or an object with custom axis keys #9601

Closed
wants to merge 8 commits into from
Closed

fix: old x axis labels were not destroyed during data update if parsing was set to false or an object with custom axis keys #9601

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
e821305
fix: Old labels were not destroyed on data update when data was not p…
Aug 30, 2021
d5cb9e6
fix: This will only destroy labels when parsing data is not primitive.
Aug 30, 2021
6cc882d
ref: Added labels cleanup earlier since old way caused issues.
Aug 30, 2021
633a52b
chore: Check if `labels` is defined.
Aug 30, 2021
7994da3
chore: Clean iScale labels instead.
CatchABus Oct 28, 2021
5a788f3
fix: Replaced undefined `me` references with `this`.
CatchABus Nov 7, 2021
ab13a9f
chore: Changed `labels` variable into constant.
CatchABus Nov 7, 2021
88ea094
test: Added tests for parsing-related labels bug fix.
CatchABus Nov 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/core/core.datasetController.js
View file
Open in desktop
Expand Up @@ -367,6 +367,11 @@ export default class DatasetController {
const dataset = this.getDataset();
let stackChanged = false;

const labels = meta.iScale ? meta.iScale.getLabels() : null;
if (labels && labels.length && (this._parsing === false || isObject(this._parsing))) {
labels.splice(0);
}

this._dataCheck();

// make sure cached _stacked status is current
Expand Down
63 changes: 63 additions & 0 deletions test/specs/core.datasetController.tests.js
View file
Open in desktop
Expand Up @@ -249,6 +249,69 @@ describe('Chart.DatasetController', function() {
expect(parsedYValues).toEqual([20, 30]);
});

it('should synchronize labels array with X values when chart existing dataset is updated and parsing is off', function() {
Copy link
Member

Choose a reason for hiding this comment

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

Does this case work properly when parsing is on, or should this fix actually apply in that case too?

Copy link
Contributor Author

@CatchABus CatchABus Nov 7, 2021 •
edited

Choose a reason for hiding this comment

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

The bug occurs when parsing is off or has provided keys, that's why I target those 2 cases.

Copy link
Contributor Author

@CatchABus CatchABus Nov 7, 2021 •
edited

Choose a reason for hiding this comment

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

@kurkle I totally missed this one but it seems issue occurs with parsing set to true as well.
Do you think I should add a 3rd test or maybe change test description to ...when parsing has no provided keys?

UPDATED: My bad. Not setting parsing inside tests defaults to false so I got confused. It seems bug does really NOT occur when parsing is set to true.

Copy link
Contributor Author

@CatchABus CatchABus Nov 7, 2021 •
edited

Choose a reason for hiding this comment

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

After few more tests, it seems I get different results for parsing: true now and this case needs to be covered.
I'm not sure if there is any cache mechanism, but unit tests suddenly failed after few more attempts.
I marked PR as draft until I resolve this.

const chart = acquireChart({
type: 'line',
data: {
datasets: [{
data: [
{x: 'One', y: 1},
{x: 'Two', y: 2}
]
}]
},
options: {
parsing: false
}
});

const newData = [
{x: 'Three', y: 3},
{x: 'Four', y: 4},
{x: 'Five', y: 5}
];

chart.data.datasets[0].data = newData;
chart.update();

const meta = chart.getDatasetMeta(0);
const labels = meta.iScale.getLabels();
expect(labels).toEqual(newData.map(n => n.x));
});

it('should synchronize labels array with X values when chart existing dataset is updated and parsing has provided keys', function() {
const chart = acquireChart({
type: 'line',
data: {
datasets: [{
data: [
{name: 'One', num: 1},
{name: 'Two', num: 2}
]
}]
},
options: {
parsing: {
xAxisKey: 'name',
yAxisKey: 'num'
}
}
});

const newData = [
{name: 'Three', num: 3},
{name: 'Four', num: 4},
{name: 'Five', num: 5}
];

chart.data.datasets[0].data = newData;
chart.update();

const meta = chart.getDatasetMeta(0);
const labels = meta.iScale.getLabels();
expect(labels).toEqual(newData.map(n => n.name));
});

it('should synchronize metadata when data are inserted or removed and parsing is on', function() {
const data = [0, 1, 2, 3, 4, 5];
const chart = acquireChart({
Expand Down