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 issue #11717 #11844

Conversation

huqingkun
Copy link
Contributor

Fixing #11717

The origin code to calculate the stack size used the data absolute index, which cause issue when the data orders are different and the skipNull is true. Change to filter by the current index id value to get count for all datasets.

Signed-off-by: Hu, Vince <Qingkun.Hu@fmr.com>
@LeeLenaleee
Copy link
Collaborator

Hi, can you add a test for this fix so that we are sure it will keep working in the future?

@huqingkun
Copy link
Contributor Author

Hi, can you add a test for this fix so that we are sure it will keep working in the future?

Sure, working on it.

@huqingkun
Copy link
Contributor Author

Hi, can you add a test for this fix so that we are sure it will keep working in the future?

Hi LeeLenaleee, I have added a new test. Could you please help to review? Thank you!

@LeeLenaleee
Copy link
Collaborator

I don't know if I have the time today so it might become tomorrow that i can take a look at it

@huqingkun
Copy link
Contributor Author

huqingkun commented Aug 2, 2024

I don't know if I have the time today so it might become tomorrow that i can take a look at it

Take your time, I just want let you know that the pr is ready for review. Btw, I noticed the lint issues and I have fixed them.

Comment on lines 444 to 445
const filtered = meta._parsed.filter(item => item[iScale.axis] === xStackIndex);
const parsed = filtered.length > 0 ? filtered[0] : undefined;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const filtered = meta._parsed.filter(item => item[iScale.axis] === xStackIndex);
const parsed = filtered.length > 0 ? filtered[0] : undefined;
const parsed = meta._parsed.find(item => item[iScale.axis] === xStackIndex);

@@ -437,9 +437,12 @@ export default class BarController extends DatasetController {
.filter(meta => meta.controller.options.grouped);
const stacked = iScale.options.stacked;
const stacks = [];
const currentParsed = this._cachedMeta.controller.getParsed(dataIndex);
const xStackIndex = currentParsed && currentParsed[iScale.axis];
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that iScaleValue might be a better name since this is not really the stack index but the value of the index scale

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that iScaleValue might be a better name since this is not really the stack index but the value of the index scale

Hi LeeLenaleee, x may not a good name. From the parsed data perspective, it's iScale value. But from the chart perspective, it's represent in which section. The 'RED' or the 'Blue' in the original issue example.
image

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would keep the code context here in mind more then how it shows on the chart itself. We read the property of the index scale which can also be the y axis. And internally it is a number and not the string you see at the chart. This conversion happens later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, going to update the codes

@huqingkun
Copy link
Contributor Author

@LeeLenaleee fixed

@LeeLenaleee LeeLenaleee added this to the 4.5.0 milestone Aug 6, 2024
@LeeLenaleee LeeLenaleee merged commit 147ee59 into chartjs:master Aug 6, 2024
8 checks passed
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.

3 participants