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

new: add XYChart proof-of-concept #745

Closed
wants to merge 45 commits into from
Closed

Conversation

williaster
Copy link
Collaborator

@williaster williaster commented Jun 12, 2020

🚀 Enhancements

This is a proof-of-concept PR for the XYChart discussed in #734, hopefully to become @vx/xy-chart. The Example is becoming somewhat overloaded / large, but it's mostly to play with and test different functionality.

Check out the sandbox link for a live demo.

image

Implemented functionality

  • ChartProvider context
    • handles data registry for Series and computes scales, considering data in the registry
  • TooltipProvider context
  • responsive dimensions
    • super performant (currently context width/height updates aren't great)
  • theming
    • default light + dark themes
    • series use context colorScale by default, which uses theme colors. can be overwritten with props
  • multiple series types
    • LineSeries
    • BarSeries
      • requires mechanism to override findNearestDatum logic
    • Stack
      • requires mechanism to override x/yDomain to account for sum of stack values
      • requires mechanism to override findNearestDatum logic
    • Group
      • requires mechanism to override findNearestDatum logic
  • animation
    • proof-of-concept animated series
      • [ ] export animated + unanimated versions I think this can be done in actual implementation
    • AnimatedAxis (should go to @vx/axis)
    • animate complex series Group
    • animate complex series Stack
  • tooltips
    • render tooltip in a Portal, uses react-use-measure for bounds detection
    • add Portal to @vx/tooltip new(vx-tooltip): add Portal + demo #755
    • uses a voronoi based algorithm to find closes datum across all series; this seems to work well for LineSeries & BarSeries need to update this UX / logic
    • tooltips get access to closestDatum, and the closestData across all series
    • series can opt out of mouse events
    • series like horizontal Bar + GroupedBar need a mechanism to override tooltip triggering logic
  • legends
  • generics for Datum, XScaleInput, and YScaleInput work correctly (partially done) I think types can be fixed in actual implementation
  • integration with @vx/brush (partially done, not in demo) will complete in actual implementation
  • integration with @vx/zoom will complete in actual implementation

@techniq @hshoff @kristw

import ChartContext from '../context/ChartContext';
import { DataRegistry } from '../types';

export default function useRegisteredData<
Copy link
Collaborator Author

@williaster williaster Jun 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one consequence of the context approach is that the data/xAccessor/yAccessor passed to a *Series may be out of sync with the xScale/yScale from context. Getting these values from context prevents this, but is an extra step in the implementation of a *Series

@techniq
Copy link
Collaborator

techniq commented Jun 12, 2020

@williaster Excellent work. The demo works really well and I am addicted to toggling between horizontal and vertical :). Going to take a bit to fully digest, but here is some initial feedback:

Voronoi for BarCharts / Time Series

I'm not sure this will work great with BarCharts (or even general TimeSeries). If you attempt to drag straight down a bar/specific time, notice how the date jumps around alot due to the closest point / voronoi. I struggled to capture an animated gif small enough to upload to Github.

image

Using the invisible BarTarget has worked well for me when there are "tracks" (BarCharts, Gnatt Charts, etc). For Area/Lines with a time scale, I typically use the following to find the closest datum (using bisect)

const { x: localX, y: localY } = localPoint(event);
  // `x` value at mouse coordinate
  const x0 = xScale.invert(localX - margin.left);
  const bisectX = bisector(x).left;
  const index = bisectX(data, x0, 1);

  const data0 = data[index - 1];
  const data1 = data[index];
  let tooltipData;
  if (findTooltipData === 'closest') {
    tooltipData =
      Number(x0) - Number(x(data0)) > Number(x(data1)) - Number(x0)
        ? data1
        : data0;
  } else if (findTooltipData === 'left') {
    tooltipData = data0;
  } else {
    tooltipData = data1;
  }

  return {
    tooltipLeft: snapToDataX ? xScale(x(tooltipData)) : localX,
    tooltipTop: snapToDataY ? yScale(y(tooltipData)) : localY,
    tooltipData
  };

I believe I based this on your findClosestDatum. I know with multi-series this gets a little tricker, as well as mixing series types (Line/Bar/etc). For band scales could we maybe create a separate voronoi within each scale.step() (the voronoi would be used to select the closest point between each series (what you underline in the demo tooltip).

Animation

Your approach for BarSeries and LineSeries works great. I do a similar approach when I animate Tree nodes/links and such, but I haven't found a way to animate more complex hierarchies without tweening the scales (especially "drilldown" transitions such as these).

Ignoring Hierarchy transitions, any thoughts on how animations might be applied to an Axis such as when you toggle Custom y-axis range or Include zero? example 1, example 2

Stack / Group charts

Curious your thoughts on an API for Grouping and Stacking. With Victory they wrap their Bars/etc with VictoryStack or VictoryGroup. Recharts adds a stackId to their Bar (and doesn't appear to support grouping). react-vis uses a stackBy on the chart/container (no example of grouping either).

It would be useful to support both Grouping and Stacking at point. Here are some d3 examples:

Shared context across charts

If I copy the <XYChart> in the demo and paste a second copy within <EventProvider> it partially works (mouse events on the second chart show the tooltip on the first).

@techniq
Copy link
Collaborator

techniq commented Jun 12, 2020

@williaster Regarding Axis animations, I got to thinking... it seems like we could use useAnimatedScale() directly within Axis? The xScale/yScale on the ChartContext is the "keyframes" we need to interpolate too (specially the scale's domain and range).

I forked the demo on codesandbox and tried to add this myself but kept running into infinite loops due to useEffect/instance equality (and the scales on context not be initialized initially) that I was struggling to overcome. There might be a better approach, but thought it was worth a shot.

@williaster
Copy link
Collaborator Author

williaster commented Jun 12, 2020

thanks for the great feedback @techniq! 🙏 going to continue to think on some of these a bit, but some initial thoughts:

Voronoi for BarCharts / Time Series

I agree the UX of this is not dialed in, it's tricky to get something that works across both the time series + "track" series, and in particular a mixture of the two as well as for horizontal "track" series 😱 . your idea of a voronoi per track in interesting, will think on that.

I also noticed that react-vis seems to deal with this with a distinction between onNearestX vs onNearestXY, which is sort of interesting. I wonder if we could add a prop that toggles how values are searched for (nearest x, y, and xy). Not sure if this should go on the XYChart or the Tooltip 🤔 probably the Chart.

Animation
any thoughts on how animations might be applied to an Axis such as when you toggle Custom y-axis range or Include zero?

Yes! I played with this and got something working with useTransition, I'm not sure useSpring(s) can work because the item keys change (may actually need to update the Animated *Series to use this too, right now keys are not changing so may not handle this). I'll push shortly 🎉

I wasn't able to get this working with a useAnimatedScales

I forked the demo on codesandbox and tried to add this myself but kept running into infinite loops due to useEffect/instance equality (and the scales on context not be initialized initially)

Two thoughts on this

  • When I tried to get useAnimatedScales working I added a .copy() when creating the scale ref here, otherwise I hit infinite loops.
  • Yes the context registry approach has this gross consequence that x/yScale are not immediately available on first render unil series data are registered 😭 I created a utility HOC withRegisteredData that handles registering data for a BaseSeriesComponent and doesn't render the series until x/yScale are available in context. We could easily create a more general withDefinedContextScales (no idea what to call this) that conditionally renders based on scale availability. Besides this caveat, I think the data registry works okay so far.

Shared context across charts

🤯 . yeah, need to make sure this works, that'd be sweet!

Stack / Group charts

I really like the Victory api where you have the Stack and Group component wrappers. I think we should get these series working in the POC (also to figure out animating them), I'll add it to the items.

@williaster
Copy link
Collaborator Author

all right the sandbox should be updated with the working AnimatedAxis 🍾 ultimately seems like this should go in @vx/axis.

not a great quality preview (I use giphy capture for these if you're looking for gifs with lots of export options):

AnimatedAxis tsx

@techniq
Copy link
Collaborator

techniq commented Jun 13, 2020

@williaster You are knocking these out quickly. Well done! Animated the Ticks/Axis lines directly and not the scale hadn't occurred to me... and the memoized useTickTransitionConfig 👍👍

I was curious where any of the animated variants would live it (didn't know if we wanted to have a react-spring dependency throughout @vx/* or just under say @vx/animated or @vx-animated/*, but then there will be a wide variety of components in there.

Btw, I noticed Render horizontally now has a multi-second delay after the Axis changes. Before it was almost a 3d effect toggling it back and forth. Not a huge issue but wanted to make you aware in case you had an idea.

Also, regarding voronoi-based tooltips (Scatterplots, etc), I've seen people recommend also using circle clipPaths to give a more "realistic" hit target for some of the outside points. Just thought I'd pass it along as I remembered it.

Thanks for the giphy tip, I'll give it a shot. I've always used kap which has a lot of options as well (does gif along with video formats), but I seem to hit the 10MB max frequently when I capture a gif long than a couple seconds.

@techniq
Copy link
Collaborator

techniq commented Jun 14, 2020

I don't want to feature creep this too much, but wanted to document some additional *Series to possibly add (I saw you added Stack and Group on the punch list above). I think the first few are fundamental, but as you go down, they become less so. Some of these could get added once we finish the PoC and merge. I can help with some of these as well, but figure it's not easy to do so until merged.

Basic series

Summary series

  • Summaries (see: react-vis and semiotic)
  • HeatmapSeries
  • HexbinSeries
  • ContourSeries
  • Trendlines (7 day moving/rolling averages, etc)
    • Probably just an example using LineSeries and the calculation. I use this myself in our apps. Here is also a d3 example and another
  • Uncertainty / Confidence

Annotations

It would be good include a plan for annotations, whether integrating with react-annotation, etc.

@williaster
Copy link
Collaborator Author

williaster commented Jun 14, 2020

^thanks for documenting these! 🙏 I agree that we could add these over time after the POC gets merged. I think functionality-wise the most important thing to flesh out in the POC is to make sure we have a solution for things like BoxPlotSeries or Uncertainty / Confidence bands whose scale domain calculation is more complicated because of potential y0/y1 values per-datum. I'll add one of these to the list above.

Some specific thoughts:

LineMarkSeries

I agree some props on LineSeries / using PointSeries under the hood would be useful 👍 (could avoid PointSeries and have MarkSeries default to <circle />marks)

Difference

Love the <Difference><Series /><Series /></Difference API 💜, this is what I used in @data-ui (Areas only, using @vx/-threshold). I like the consistency it has with Group and Stack 👌

Animated Series

What do you think about exporting both animated and non-animated series, versus only animated series? My concern with 2 sets is a lot of duplicate code, but could try to factor out the key pieces to enable opitonally having react-spring in your bundle.

(also, need to figure out the Axis perf thing you commented on 🤔 )


Note on tooltip/mouse detection

In thinking about this a little more, I'm wondering if we could solve the mouse UX problem varying by Series type, by adding a hook to override computing this in the dataRegistry.

e.g., Bars would know if they need to use the track strategy, and they also know if they're rendered horizontally (in which case you need horizontal tracks), and Lines etc. could use voronoi.


Updates (now in sandbox)

  • I got Portal / renderInPortal working for Tooltip 🎉

  • I wanted to get Series colors to work with the theme, so that default behavior of series is decent (otherwise a theme really is only useful for Axis styles). I got this working with a ChartProvider-provided colorScale, which uses { domain: dataKeys, range: theme.colors } by default (but allows users to override domain if they want to specify the order). colorScale could be used by default in Series, but could be overridden at the Series props level. Does this seem reasonable?

@williaster
Copy link
Collaborator Author

Oops, re Annotations, I agree a plan would be good. iirc react-annotations has a very json-y config, will revisit and circle back.

@techniq
Copy link
Collaborator

techniq commented Jun 14, 2020

I mostly mention all those to help flush out the API.

Series

I like the idea of a generic MarkSeries that defaults to circle, and possible adding a mark prop to LineSeries.

Difference: We think alike.. I never noticed this in data-ui (I mostly would look at data-ui for inspiration but used vx directly.

Animated series

I definately think we need non-animated versions as well. It would be nice if you could always pass in a AnimatedVersion as a prop to the base/non-animated version, but not sure if it will always work that way in practice. We discussed providing a renderProp version for most of our shapes a while back, but renderProps with xy-chart doesn't seem like the right approach.

in short, yes we should have both versions, and try to find the best api. I like providing a lot of animated versions "out of the box" but allowing custom versions. Where they live (to reduce react-spring dependency, bundle size, etc) is debatable.

Tooltips

I saw, nice work! Regarding the strategy per series type, I think that sounds great. What happens when you mix series types though (like our example of with a BarSeries and LineSeries, or if we added in a MarkSeries to the example). Would the order defined dictate the precedence? Would we try to come to some kind of consensus based on each series?. If we used the voronoi with circle cropping, we might be able to overlap the BarSeries targets as the "base" layer, and the voronoi with some maximum proximity circle on top... so in practice, you would get the Bar target unless you were within range of one of the points (cropped by voronoi). Not sure how this would work in practice if the lines overlap the bars too much (you may never be able to get to the Bar. Maybe you can disable the targets per series as well (maybe you just want the "tracks" for a BarSeries or time-based LineSeries, although the datum for all the series would be selected based on the closest point (using bisector)? Definitely need to explore some, but I do think there isn't a one size fits all so needs to be flexible.

Color Scales

Makes sense to me. I do something similar in our charts, but with a lot of customization based on use case. One of those things I would have to feel out in practice (and probably provide examples / documentation) but I think it sounds like a sane approach.

@techniq
Copy link
Collaborator

techniq commented Jun 14, 2020

A few more quick thoughts (sorry)...

Data-only Series

Since a *Series is providing 2 uses (adding data to the chart, and showing it visually), what about if we wanted to only add data, and not show it visually. I would think we could add a hidden (or similar) prop to all our series, or have a DataSeries (or just Series). Does that make sense?

One use for this would be a BumpSeries, where it takes in all the data and "ranks` it based on the overall rank within the all the other series. A few examples:

Other cases is when we just want the "summary" series shown (Trendline/Moving average, Heatmap, etc) or optionally turns off/on (could conditionally render the *Series component, but then the data would be de-registered.

Parallel Coordinates

Basically adding multiple Axis components across the chart, but real power is with brushing (across each axis) and drag and drop axis location

Ridgeline / Small multiples examples

BulletChart

  • Multiple BarSeries laid on top of one another, with a mark/line. Might need any special handling, but trying to identify chart types we can just compose, vs needing additional support
  • d3 example

Playground

It would be useful to have a "playground" to play around with the different components and such. I think initially having a codesandbox template we can easily fork that includes all the (latest) vx packages with a basic chart examples would be a good start, but later adding conviencines to supply your own data, etc.

I'm experimenting with this with my hierarchy examples over the coming week (giving some example data sets but also allowing you to specify a json structure, or CSV for both hierarchy and graph (node/links).

@kristw
Copy link
Collaborator

kristw commented Jun 15, 2020

Data-only Series

Since a *Series is providing 2 uses (adding data to the chart, and showing it visually), what about if we wanted to only add data, and not show it visually. I would think we could add a hidden (or similar) prop to all our series, or have a DataSeries (or just Series). Does that make sense?

One use for this would be a BumpSeries, where it takes in all the data and "ranks` it based on the overall rank within the all the other series. A few examples:

I think making XYChart handling these data processing logics will be too complicated though. For bump charts you could compute the ranking outside and pass it to line series.

series1 = [{ time: 1, rank: 1}, { time: 2, rank: 1}, ...]
series2 = [{ time: 1, rank: 2}, { time: 2, rank: 3}, ...]

Parallel Coordinates
Basically adding multiple Axis components across the chart, but real power is with brushing (across each axis) and drag and drop axis location

I object the idea of supporting parallel coordinates.

  • There is a fundamental difference between each line in parallel coordinate and line charts. Each line in a parallel coordinates is a single data point. While each line in a line chart is an array of data points.

    • parallel coordinates: { originalPrice: 20000, make: 'Honda', type: 'sedan', sold: 14000 } => a line with 4 points.
    • line: [{time: 1, price: 20000}, {time:2, price 21000}, {time:3, price 22000}] => line with 3 points.
  • This will make each XXXSeries having to handle n scales passed to them instead of two.

  • If you really want to handle multi-axes. It can be a separate chart which support different alignment of axes IMO and handle each data point as a line.

https://pure.tue.nl/ws/files/3493590/687784949356560.pdf

Ridgeline / Small multiples examples

Faceting is probably something that can build as another layer on top of XYChart rather than inside it. vega-lite has 3 mode of faceting (horizontal, vertical and grid)

@techniq
Copy link
Collaborator

techniq commented Jun 16, 2020

@kristw That all makes sense. Thanks!

@tgdn
Copy link

tgdn commented Jun 17, 2020

The sandbox doesn't seem to work?

image

@williaster
Copy link
Collaborator Author

@tgdn sorry that will be broken until we publish some of the changes needed in other packages in 0.0.198

@tgdn
Copy link

tgdn commented Jun 24, 2020

No problem. Got the code anyway 😊 thanks!

height,
});

const nearestDatum = voronoiInstance(data).find(svgMouseX, svgMouseY);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it expensive to create this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't done profiling but it feels performant in the UI. this is what react-vis uses.

};

export interface ChartContext<
Datum = unknown,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be { [key in DataKeys]: unknown } ?
May have to change order of the generic type.

@williaster
Copy link
Collaborator Author

I think I have everything working (animated stack/group with positive / negative values, overrides for tooltip positioning logic [findNearestDatumX/Y/XY])). I need to publish 0.0.198 to get the sandbox working, and will write up some learnings in the next couple of days.

@williaster
Copy link
Collaborator Author

Okay, after a long period of silence. 0.0.198 is out and the sandbox is working again 🎉 The way I am thinking about this is that we can close the POC and I'll move toward more small, higher-quality PRs for the real @vx/xy-chart package with better types and refined API. But first I'd like to recap some updates to get feedback on them.

Expanded data registry

  • Series must minimally register key, data, and x/yAccessors (as discussed above)
  • I found that more complex Series also need hooks to update the logic for tooltips, and make x/yScale modifications. Therefore they may also optionally register
    • a findNearestDatum(...) function to override the logic for finding the nearest Series datum for a mouse/touch event.
    • a x/yScale => x/yScale function to apply any scale updates

Data registry clunkiness

  • Series are passed data props, which they register with the ChartProvider dataRegistry in context. With this model, issues occur when a Series attempts to use props.data and context.x/yScale because the two may be out of sync. This can be avoided by always using context.dataRegistry[key].data, but it's a bit clunky and error prone.

Curious if there are thoughts for how to improve this consequence of the architecture.

StackedBarSeries API / implementation

<Stack>
  <Bar key="a" data={...} />
  <Bar key="a" data={...} />
</Stack>

Stack

  • registers its child BarSeries keys and data (this works well with the legend and tooltip which use keys for nearest datum and legend items)
  • must update the yDomain (xDomain if horizontal) to include the signed (+/-) sums of the child data, since the stack represents the sums
  • must override findNearestDatum logic:
    • first it uses findNearestDatumX/Y (depending on horizontal)
    • after finding the nearest stack, it then finds the nearest bar within the stack with additional logic

GroupedBarSeries API / implementation

<Group>
  <Bar key="a" data={...} />
  <Bar key="a" data={...} />
</Group>

Group

  • same as Stack, it registers its child BarSeries keys and data
  • must override findNearestDatum logic:
    • first it uses findNearestDatumX/Y (depending on horizontal)
    • after finding the nearest group, it then finds the nearest bar within the group with additional logic / using its encapsulated band scale

Mouse events / tooltips

  • currently a rect overlay is used in XYChart to capture mouse events and uses findNearestData(event) provided in context to locate the nearest datum per-series, and the overall closest datum
  • Series can optionally pass a findNearestDatum when registering data in context, which is then used by findNearestData. This defaults to nearest XY (using a voronoi), but Bar/StackedBar/GroupedBar use nearest X or Y (if rendered horizontally)
    • I think a single event layer is useful as it removes the need for rendering specialized "tracks," it's unclear to me how these should work when rendering multiple series types (e.g., a line on top of a bar)
  • there might be an opportunity to flesh out the event emitters described in Shared event delegation  #761 which harry has a POC for

Copy link
Collaborator

@kristw kristw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for all the hard work. I think this looks more and more realistic.

useRegisteredData

To avoid calling useRegisteredData() hook in every Series file, what if we make withRegisteredData() swap the data and scales that are actually passed to the component?

function withRegisteredData(...) {
  // ...
  return xScale && yScale && dataRegistry?.[dataKey]?.data === data ? (
    // Change the props here to pass scales and data from registry?
    <BaseSeriesComponent {...props} />
  ) : null;
}

Storing combined data in the context

Another concern I have overall is how registerData() currently combine data from all series (eventhough they are the exact same array). With 20 <XXXSeries data={currData} />, the context ends up storing 20 concatenations of currData.

In packages/vx-demo/src/sandboxes/vx-chart-poc/src/components/providers/ChartProvider.tsx, can the state be stored more efficiently, particularly dataRegistry and combinedData fields?

Would something like a Map<Dataset, Set<DataKey>> work?
(Use approach similar to reference counting. Using the Map type, we can use non-primitive as a key)

  • When a Dataset is registered, if the dataset does not exist in the map, map.set(Dataset, [key]. If exist, just append the datakey to existing list/set of keys.
  • When the Dataset is unregistered, remove DataKey (string). If there are still remaining keys, keep the Dataset. Otherwise, remove Dataset from map.

What happen if data is updated?

I am not sure what happen if data changes, e.g. for real-time chart use-chase? Does the context get updated?

Comment on lines +70 to +71
closestDatum: DatumWithKey;
closestData: { [dataKey: string]: DatumWithKey };
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are the difference between closestDatum and closestData?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good question: closestDatum is the single closest data point across all series, closestData contains the closest data point for each series (useful for shared tooltips)

Comment on lines +77 to +78
svgMouseX: number;
svgMouseY: number;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could these be shortened and called svgX and svgY?

const domainIndex = bisectLeft(rangePoints, mouseCoord);
// y-axis scales may have reverse ranges, correct for this
const sortedDomain = range[0] < range[1] ? domain : domain.reverse();
const domainValue = sortedDomain[domainIndex - 1];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can do stringify once before instead of in the loop

const domainValue = String(sortedDomain[domainIndex - 1]);

import { bisector, range as d3Range, bisectLeft } from 'd3-array';
import { ScaleType } from '../types';

export default function findNearestDatumSingleDimension<Datum, ScaleInput>({
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

findNearestDatum1D?

import { NearestDatumArgs } from '../types';

// finds the datum nearest to svgMouseX/Y using voronoi
export default function findNearestDatumXY<
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

findNearestDatum2D?

@@ -0,0 +1,3 @@
export default function isValidNumber(_: unknown): _ is number {
return _ != null && typeof _ === 'number' && !isNaN(_) && isFinite(_);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use Number.isNaN and Number.isFinite


/** Removes data from the registry and combined data. */
unregisterData = (keyOrKeys: string | string[]) => {
const keys = new Set(typeof keyOrKeys === 'string' ? [keyOrKeys] : keyOrKeys);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const keys = Array.isArray(keyOrKeys) ? [...new Set(keyOrKeys)] : [keyOrKeys];

const dataRegistry = { ...state.dataRegistry };
keys.forEach(key => delete dataRegistry[key]); 

or can also push filter out of reduce block

Object.entries(state.dataRegistry).filter(([key, value]) => !keys.has(key)).reduce(...);

theme,
xScale: xScaleConfig,
yScale: yScaleConfig,
colorScale: colorScaleConfig,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assumes there is only one color scale for the entire chart.
What if there are multiple color scales?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you describe a use case you're thinking of? I'm not sure of a great way to support n color scales and know which to use where. maybe encodable could provide some ideas?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

umm, maybe i'm overthinking. Do you meant for this color scale to be used only for coloring series by its key, e.g. each line in line chart?

    const colorScale = scaleOrdinal({
      domain: Object.keys(dataRegistry),
      range: theme.colors,
      ...colorScaleConfig,
    });

{/** @ts-ignore */}
<ChartProvider
theme={themeObj}
xScale={renderHorizontally ? temperatureScaleConfig : dateScaleConfig}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could the vertical/horizontal flipping be built in as part of the provider somehow?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is an interesting idea. It seems a little counter intuitive to me to explicitly set x/yScale={...}, and then have them be flipped by setting a horizontal parameter. I think it's more intuitive to have the user specify this instead? It might look complicated in the example, but the example is trying to show all of the features 😝

as far as horizontal flags for e.g., BarSeries, I think that also needs to be set explicitly because each series has it's own calculations it needs to do for horizontal vs vertical rendering.

}: DataRegistry[string]) {
const { registerData, unregisterData } = useContext(ChartContext);

// register data on mount
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if the data is updated?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when data is updated, since it's a useEffect dependency the return value of useEffect is called so it's unregistered, then re-registered with the new data.

I think there's room for some optimizations here somehow, two sequential registry updates is potentially pretty expensive.

@williaster
Copy link
Collaborator Author

closing this as it was a proof-of-concept to facilitate discussion. follow the xychart project for production-ized updates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants