-
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
Use tick.major rather than recomputing #6265
Conversation
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.
Lgtm
src/scales/scale.time.js
Outdated
@@ -408,8 +408,9 @@ function computeOffsets(table, ticks, min, max, options) { | |||
return {start: start, end: end}; | |||
} | |||
|
|||
function ticksFromTimestamps(scale, values, majorUnit) { | |||
function ticksFromTimestamps(scale, values) { |
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'm not fan of this change because it makes the code harder to maintain by obfuscating the method inputs. Except for _adapter
, I wouldn't assume any of the other scale computed values to be available at the time of calling this method.
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.
Not sure if we care about the size, but the way I wrote it is 6 bytes smaller
Thanks for the review @simonbrunel. I've updated the PR |
@benmccann Can you rebase this PR? |
fd9aa32
729aa1e
to
fd9aa32
Compare
Thanks. I've rebased this PR |
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.
Thanks @benmccann for the changes!
getLabelForIndex
is ignoring the value oftick.major
and recomputing it, which means you can't use your own computation oftick.major
inafterBuildTicks