-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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
fix issue #11717 #11844
Conversation
Signed-off-by: Hu, Vince <Qingkun.Hu@fmr.com>
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. |
Hi LeeLenaleee, I have added a new test. Could you please help to review? Thank you! |
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. |
src/controllers/controller.bar.js
Outdated
const filtered = meta._parsed.filter(item => item[iScale.axis] === xStackIndex); | ||
const parsed = filtered.length > 0 ? filtered[0] : undefined; |
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.
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); |
src/controllers/controller.bar.js
Outdated
@@ -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]; |
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.
I think that iScaleValue
might be a better name since this is not really the stack index but the value of the index scale
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.
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.
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.
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.
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.
Sure, going to update the codes
@LeeLenaleee fixed |
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.