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

How to handle scaleSequential sales with interpolators? #73

Closed
mhkeller opened this issue May 13, 2022 · 4 comments
Closed

How to handle scaleSequential sales with interpolators? #73

mhkeller opened this issue May 13, 2022 · 4 comments
Labels
awaiting-merge Finished in a branch, will be included in next release
Milestone

Comments

@mhkeller
Copy link
Owner

mhkeller commented May 13, 2022

By default, LayerCake sets a range for each of the scales. This means that you shouldn't set ranges using the d3 shorthand xScale={scaleBand([0, 100])} but instead set these two props separately

xScale={scaleBand()}
xRange={[0, 100]}

This works fine except for color scales that need to be set via an interpolator method like this as described by d3-scale-chromatic:

const piyg = d3.scaleSequential().interpolator(d3.interpolatePiYG);
// or...
const piyg = d3.scaleSequential(d3.interpolatePiYG);

This could be solved by adding xInterpolator, yInterpolator, zInterpolator and rInterpolator props. Maybe that's too many props?

@mhkeller mhkeller added the discussion Talk about implementation of different chart types. label Jun 30, 2022
@mhkeller mhkeller modified the milestones: v7, 6.2 Jul 5, 2022
@mhkeller
Copy link
Owner Author

mhkeller commented Jul 9, 2022

Maybe the easiest thing to do is to detect whether the passed in range is a function and if so, don't set .range on the scale and instead set .interpolator. This is complicated by the fact that range can already be a function that passed in width and height...

@mhkeller
Copy link
Owner Author

mhkeller commented Jul 9, 2022

The syntax for this would be

<LayerCake
  zRange={() => interpolateRdGy}

and internally in createScale.js

if (typeof defaultRange === 'function') {
  scale.interpolator(defaultRange)
} else {
  scale.range(defaultRange);
}

I don't like that you would have to know to set zRange to a function in order to use an interpolator. I think I would rather get rid of the functionality that allows for range props to be functions since there isn't a great use case for that – just seemed like a possible option for down the road...

@mhkeller
Copy link
Owner Author

mhkeller commented Jul 9, 2022

This could be a better way to do it. Set an interpolator on the scale itself

<LayerCake
  zScale={scaleSequential(interpolateRdGy)}
...or
  zScale={scaleSequential().interpolator(interpolateRdGy)}

And then in layercake, only override the range if there is no scale interpolator function, or if there is one, that is the default identity function. This would allow the user to pass in a custom zRange using a scaleSequential or scaleDiverging

if (!scale.interpolator || (typeof scale.interpolator === 'function' && scale.interpolator().name.startsWith('identity'))) {
  scale.range(defaultRange);
}

Need to add documentation explaining that _Range props are ignored if the scale has an interpolator function set.

Open to any thoughts!

@mhkeller mhkeller added awaiting-merge Finished in a branch, will be included in next release and removed discussion Talk about implementation of different chart types. labels Jul 9, 2022
@mhkeller
Copy link
Owner Author

This is added in v7.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-merge Finished in a branch, will be included in next release
Projects
None yet
Development

No branches or pull requests

1 participant