-
Notifications
You must be signed in to change notification settings - Fork 33
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
Add deriveScales
prop
#210
Conversation
@techniq if this looks good, i'll merge this in after i do the docs |
TBH I haven't gotten a chance to kick it around, but the approach looks great and I say ship it. I'm looking to leverage it at some point within LayerChart, hoping to simplify createDimensionGetter and hopefully improve tooltips for group/stacked data |
Any chance this will be merged and released in the next day or two? I'm currently overhauling how I handle groups and stacks in LayerChart and would love to leverage this for grouping. |
I wanted to get a couple other eyes on it first to make sure I got the reactivity correct but that hasn't come through yet. Are you able to either |
I always forget about installing via a git branch. I just tried via I'm OK with waiting for now as I'd like to release the new simplified charts hopefully in the next 2 weeks, and wouldn't want to have a local-only dependency holding things up (or break the CI PR build). |
Just had a thought, if you would want to publish the package as a |
@techniq Try installing |
Just had a quick minute to install, and looks like I'm in business! I'll start kicking it around this week and let you know how it goes (I'm pretty eager to see how well this cleans up some things, namely createDimensionGetter, as moving this up to the context will be much cleaner). Thanks for the PR and pushing the beta package. |
Fantastic. Let me know what you're seeing particularly around any weird re-renders and API ergonomics. |
Hey @mhkeller, had a little bit of time to play around with this, and ran into some issues / have some suggestions.
As with your example, the only way to associate an accessor with a derived scale is to use/consume another scale such as <LayerCake
x="group"
xScale={scaleBand().paddingInner(0.1).round(true)}
r="subgroup"
rScale={scaleBand().paddingInner(0).round(true)}
deriveScales={{
subroupScale: ({ xScale, rScale }) => {
rScale.range([0, xScale.bandwidth()]);
return rScale;
}
}}
> This will make it hard in practice to have more than 1 or 2 derived scales (although I'm not sure how often that will truly be needed), but it also has a tight coupling between 2 scales. Instead of creating any number of derived scales, I think it might be better to have a single derived scale for each of the current scales ( <LayerCake
{data}
x="group"
xScale={scaleBand().paddingInner(0.1).round(true)}
x1="subgroup"
x1Scale={scaleBand().paddingInner(0).round(true)}
x1Domain={new Set(...data.map(d => d.subgroup))}
x1Range={xScale => [0, xScale.bandwidth()])}
> This was basically the idea in this comment), but maybe we don't need to bother with some the extra conventions mentioned ( Thoughts? |
Hey @mhkeller, I found a way to implement You can see the new scales in practice by checking out a few of the column and bar examples, but where it really shines, is enabling My plan is to continue to leverage At some point I would love to consolidate/simplify more logic currently spanned between LayerChart's <Chart>
<LayerCake>
<ChartContext>
<slot />
</ChartContext>
</LayerCake>
</Chart> |
Very neat! So I think based on your tests, this approach is generally not the way to go. I'll close this PR as I rethink what the best API is. |
I wonder what the best thing to do about the beta version on npm is. I may end up publishing version 8.0 where extents is deprecated and domain sort order default is changed. |
Adds a prop that lets the user create a new scale based off one or more existing scales.
Usage creating a new scale called
subroupScale
that is the same as the rScale but uses the bandwidth of the xScale as part of its range.