Adding invert method to ordinal scale#1046
Conversation
|
|
mcnuttandrew
left a comment
mcnuttandrew
left a comment
There was a problem hiding this comment.
🎉!!!
This is wonderful! Thanks for doing this. I have a bunch of code style comments, but substantively this looks great. You're a champ
src/utils/scales-utils.js
Outdated
| scale.invert = function invert(value) { | ||
| let domainIndex = 0; | ||
| const n = scale.domain().length; | ||
| const reverse = scale.range()[1] < scale.range()[0]; |
There was a problem hiding this comment.
maybe cleaner to pull the start/end values out rather than to keep calling range()
There was a problem hiding this comment.
Not completely sure what do you mean here; we need to figure out whether range is ascending or descending in order to see what is 'start' (lower range) and 'end' (higher range).
There was a problem hiding this comment.
you call scale.range() fourish times, it might be better to do something like:
const [lower, upper] = scale.range();
const start = Math.min(lower, upper);
const end = Math.max(lower, upper);That way there is less index manipulation
src/utils/scales-utils.js
Outdated
| const start = scale.range()[reverse - 0]; | ||
| const stop = scale.range()[1 - reverse]; | ||
|
|
||
| if (value < start + scale.padding() * scale.step()) domainIndex = 0; |
There was a problem hiding this comment.
I'm pretty surprised that lint isn't complaining about this, but we prefer using explicit curly braces on if statements.
src/utils/scales-utils.js
Outdated
| else if (value > stop - scale.padding() * scale.step()) domainIndex = n - 1; | ||
| else domainIndex = Math.floor((value - start - scale.padding() * scale.step()) / scale.step()); | ||
|
|
||
| return scale.domain()[domainIndex]; |
There was a problem hiding this comment.
rather than updating the domainIndex in each of the branches, why don't you just return the scale.domain()[VALUE] from one of the arms of the conditional?
| 'should build a generic domain that reflects about zero' | ||
| ); | ||
|
|
||
| const ordinalScale = getScaleFnFromScaleObject({ |
There was a problem hiding this comment.
maybe add a test for this that also capture padding? or does this do that already?
There was a problem hiding this comment.
We are already testing padding of 0.5 (which is padding we use in react-vis).
This is so-called 'outer' padding (padding before first bar and after last bar).
Inner padding is set to 1.0 in d3-scale, as we are really using 'point-scale' from d3-scale.
src/utils/scales-utils.js
Outdated
| */ | ||
|
|
||
| function addInvertFunctionToOrdinalScaleObject(scale) { | ||
| if (scale.invert) return; |
There was a problem hiding this comment.
curly brace here too?
mcnuttandrew
left a comment
mcnuttandrew
left a comment
There was a problem hiding this comment.
Thanks again for doing this!
|
Hey, no problem :-) Thanks for your help! PS. I closed #987 |
3 participants
Ordinal scale does not have 'invert' method implemented in d3-scale, thus we are not able to zoom charts with ordinal axes using
Highlightcomponent.I opened a pull request to d3-scale (d3/d3-scale#151); however as d3-scale is slow moving I had to implement temporary workaround.