-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[charts] Improve grouped axis logic #19069
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
Conversation
Deploy preview: https://deploy-preview-19069--material-ui-x.netlify.app/ Bundle size report
|
CodSpeed Performance ReportMerging #19069 will improve performances by 9.12%Comparing Summary
Benchmarks breakdown
Footnotes |
labelOffset, | ||
}); | ||
|
||
if (!storedOffsets.has(tickOffset)) { |
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.
Would it be feasible to rely on the dataIndex
to avoid using floating point number as a key?
This part of the code is quite hard to get. Especially because storedOffsets
is not very explicit. Whay those offset are stored?
Maybe something like offsetsToTickIndexes
/dataIndexToTickIndes
with a comment
// Map the axis dataIndexes to the tick indexes related.
const dataIndexToTicks = new Map<number, Set<number>>();
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.
We can't use dataIndex
in this case, we need to know exactly where the tick is, as this is used to remove duplicate ticks.
Floats should be fairly consistent overall, they are just not precise. Can you see an issue that might be created by this?
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.
Can you see an issue that might be created by this?
I don't get why we need the position and not the dataIndex.
The tickOffset
is computed from the scale
, the tickValue=tickValues[dataIndex]
, and tickPlacement
Either tickPlacement
is the same for all groups. In that case, same tickOffset
is equivalement to same dataIndex
. Or tickPlacement
can change from one group to another. But in that case it seems risky to rely on the fact that tickPlacement='start'
of an item and tickPlacement='end'
of the previous item get exactly the same floating number
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.
Ah true, because the ticks are always in the same place 🤦
I'll change it
Or tickPlacement can change from one group to another. But in that case it seems risky to rely on the fact that tickPlacement='start' of an item and tickPlacement='end' of the previous item get exactly the same floating number
As long as the original numbers are the same the results should be the same.
|
||
// Calculate the label offset | ||
const labelOffset = | ||
(scale as { step: () => number }).step() * |
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.
Should we update the scale
type to be D3DiscreteScale
(and create that type) instead?
Fixes #19067