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

Add deriveScales prop #210

Closed
wants to merge 6 commits into from
Closed

Add deriveScales prop #210

wants to merge 6 commits into from

Conversation

mhkeller
Copy link
Owner

@mhkeller mhkeller commented Jun 10, 2024

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.

deriveScales={{
  subroupScale: ({ xScale, rScale }) => {
    rScale.range([0, xScale.bandwidth()]);
    return rScale;
  }
}}

@mhkeller mhkeller marked this pull request as draft June 10, 2024 15:06
@mhkeller mhkeller marked this pull request as ready for review June 22, 2024 21:47
@mhkeller
Copy link
Owner Author

@techniq if this looks good, i'll merge this in after i do the docs

@techniq
Copy link
Contributor

techniq commented Jun 22, 2024

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

@techniq
Copy link
Contributor

techniq commented Aug 27, 2024

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.

@mhkeller
Copy link
Owner Author

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 npm link to this branch or do npm install mhkeller/layercake#derive-scales to install from this branch so you can develop locally?

@techniq
Copy link
Contributor

techniq commented Aug 28, 2024

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 npm link to this branch or do npm install mhkeller/layercake#derive-scales to install from this branch so you can develop locally?

I always forget about installing via a git branch. I just tried via pnpm add mhkeller/layercake#derive-scales but ran into issues right out the gate...

image

image

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).

@techniq
Copy link
Contributor

techniq commented Aug 28, 2024

Just had a thought, if you would want to publish the package as a next version I could try it that way.

@mhkeller
Copy link
Owner Author

@techniq Try installing 8.4.0-beta.1

@techniq
Copy link
Contributor

techniq commented Aug 28, 2024

@techniq Try installing 8.4.0-beta.1

Just had a quick minute to install, and looks like I'm in business!

image

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.

@mhkeller
Copy link
Owner Author

Fantastic. Let me know what you're seeing particularly around any weird re-renders and API ergonomics.

@techniq
Copy link
Contributor

techniq commented Aug 31, 2024

Hey @mhkeller, had a little bit of time to play around with this, and ran into some issues / have some suggestions.

  • No accessor associated for derived scales (must leverage another scale such as r or z, as you did in your example)
  • Difficult to know which derived scales MIGHT be defined, which is more of a specific issue for LayerChart but could be solved with conventions (expect derivedScale={{ x1Scale: ... }})
  • You can create ANY number of derived scales based on ANY "standard" scale, but in practice I think there is a coupling between a single derived scale and a single standard scale

As with your example, the only way to associate an accessor with a derived scale is to use/consume another scale such as r/rScale. In a lot of ways you are just converting the rScale to a derived scale, than creating a new one.

<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 (xScale, yScale, zScale, rScale), and just pass that scale as a parameter to x1Domain, x1Range, etc.

<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 (x1Scale defaults to scaleBand() if xScale={scaleBand}.

Thoughts?

@techniq
Copy link
Contributor

techniq commented Sep 15, 2024

Hey @mhkeller, I found a way to implement x1 and y1 scales in LayerChart by leveraging it's <ChartContext>, which I added to improve context types via chartContext() which extends getContext('LayerCake') but also has better typing, and gives access to LayerCake captured values like width and height, which is needed for the x1/y1 scales as well as a resize event we added recently (for some force visualization use cases).

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 seriesLayout="group" for BarChart.

My plan is to continue to leverage <ChartContext> where extensions are needed, for example adding a new cScale for colors (instead of (ab)using the rScale), and work through the other Chart improvements ideas.

At some point I would love to consolidate/simplify more logic currently spanned between LayerChart's <Chart>/<ChartContext> and LayerCake's <LayerCake> components where it makes sense (move functionality up/down this stack).

<Chart>
  <LayerCake>
    <ChartContext>
      <slot />
    </ChartContext>
  </LayerCake>
</Chart>

@mhkeller
Copy link
Owner Author

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.

@mhkeller
Copy link
Owner Author

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.

@mhkeller mhkeller closed this Sep 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants