new: add XYChart proof-of-concept
#745
Conversation
| import ChartContext from '../context/ChartContext'; | ||
| import { DataRegistry } from '../types'; | ||
|
|
||
| export default function useRegisteredData< |
There was a problem hiding this comment.
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
|
@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 SeriesI'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.
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 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 AnimationYour 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 Stack / Group chartsCurious your thoughts on an API for Grouping and Stacking. With Victory they wrap their Bars/etc with VictoryStack or VictoryGroup. Recharts adds a It would be useful to support both Grouping and Stacking at point. Here are some d3 examples:
Shared context across chartsIf I copy the
|
|
@williaster Regarding Axis animations, I got to thinking... it seems like we could use 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. |
|
thanks for the great feedback @techniq! 🙏 going to continue to think on some of these a bit, but some initial thoughts:
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 I also noticed that
Yes! I played with this and got something working with I wasn't able to get this working with a
Two thoughts on this
🤯 . yeah, need to make sure this works, that'd be sweet!
I really like the |
|
all right the sandbox should be updated with the working not a great quality preview (I use giphy capture for these if you're looking for gifs with lots of export options):
|
|
@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 I was curious where any of the animated variants would live it (didn't know if we wanted to have a Btw, I noticed 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 |
|
I don't want to feature creep this too much, but wanted to document some additional Basic series
Summary series
AnnotationsIt would be good include a plan for annotations, whether integrating with react-annotation, etc. |
|
^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 Some specific thoughts:
I agree some props on
Love the Animated SeriesWhat 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 (also, need to figure out the Axis perf thing you commented on 🤔 ) Note on tooltip/mouse detectionIn 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 e.g., Updates (now in sandbox)
|
|
Oops, re |
|
I mostly mention all those to help flush out the API. SeriesI like the idea of a generic
Animated seriesI definately think we need non-animated versions as well. It would be nice if you could always pass in a 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. TooltipsI 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 Color ScalesMakes 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. |
|
A few more quick thoughts (sorry)... Data-only SeriesSince a One use for this would be a
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 Parallel CoordinatesBasically adding multiple
Ridgeline / Small multiples examples
BulletChart
PlaygroundIt 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). |
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}, ...]
I object the idea of supporting parallel coordinates.
https://pure.tue.nl/ws/files/3493590/687784949356560.pdf
Faceting is probably something that can build as another layer on top of XYChart rather than inside it. |
|
@kristw That all makes sense. Thanks! |
|
The sandbox doesn't seem to work?
|
|
@tgdn sorry that will be broken until we publish some of the changes needed in other packages in |
|
No problem. Got the code anyway 😊 thanks! |
williaster
force-pushed
the
chris--xychart-poc
branch
from
7618b7d
to
61c6fdf
Compare
| height, | ||
| }); | ||
|
|
||
| const nearestDatum = voronoiInstance(data).find(svgMouseX, svgMouseY); |
There was a problem hiding this comment.
Is it expensive to create this?
There was a problem hiding this comment.
I haven't done profiling but it feels performant in the UI. this is what react-vis uses.
| }; | ||
|
|
||
| export interface ChartContext< | ||
| Datum = unknown, |
There was a problem hiding this comment.
Can this be { [key in DataKeys]: unknown } ?
May have to change order of the generic type.
|
I think I have everything working (animated stack/group with positive / negative values, overrides for tooltip positioning logic [ |
…tumX, findNearestDatumY, hoook up to Group
…try.data, misc fixes
williaster
force-pushed
the
chris--xychart-poc
branch
from
3197be5
to
855313c
Compare
|
Okay, after a long period of silence. Expanded data registry
Data registry clunkiness
❗ 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>
GroupedBarSeries API / implementation <Group>
<Bar key="a" data={...} />
<Bar key="a" data={...} />
</Group>
Mouse events / tooltips
|
There was a problem hiding this comment.
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
Datasetis 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
Datasetis unregistered, removeDataKey(string). If there are still remaining keys, keep theDataset. Otherwise, removeDatasetfrom 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?
| closestDatum: DatumWithKey; | ||
| closestData: { [dataKey: string]: DatumWithKey }; |
There was a problem hiding this comment.
What are the difference between closestDatum and closestData?
There was a problem hiding this comment.
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)
| svgMouseX: number; | ||
| svgMouseY: number; |
There was a problem hiding this comment.
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]; |
There was a problem hiding this comment.
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>({ |
| import { NearestDatumArgs } from '../types'; | ||
|
|
||
| // finds the datum nearest to svgMouseX/Y using voronoi | ||
| export default function findNearestDatumXY< |
| @@ -0,0 +1,3 @@ | |||
| export default function isValidNumber(_: unknown): _ is number { | |||
| return _ != null && typeof _ === 'number' && !isNaN(_) && isFinite(_); | |||
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
This assumes there is only one color scale for the entire chart.
What if there are multiple color scales?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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} |
There was a problem hiding this comment.
Could the vertical/horizontal flipping be built in as part of the provider somehow?
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
What if the data is updated?
There was a problem hiding this comment.
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.
|
closing this as it was a proof-of-concept to facilitate discussion. follow the |
🚀 Enhancements
This is a proof-of-concept PR for the
XYChartdiscussed in #734, hopefully to become@vx/xy-chart. TheExampleis becoming somewhat overloaded / large, but it's mostly to play with and test different functionality.Check out the sandbox link for a live demo.
Implemented functionality
ChartProvidercontextSeriesand computes scales, considering data in the registryTooltipProvidercontextcolorScaleby default, which uses theme colors. can be overwritten with propsLineSeriesBarSeriesfindNearestDatumlogicStackx/yDomainto account for sum of stack valuesfindNearestDatumlogicGroupfindNearestDatumlogic[ ] export animated + unanimated versionsI think this can be done in actual implementationAnimatedAxis(should go to@vx/axis)GroupStackPortal, usesreact-use-measurefor bounds detectionPortalto@vx/tooltipnew(vx-tooltip): add Portal + demo #755uses aneed to update this UX / logicvoronoibased algorithm to find closesdatumacross all series; this seems to work well forLineSeries&BarSeriesclosestDatum, and theclosestDataacross all seriesBar+GroupedBarneed a mechanism to override tooltip triggering logiccolorScale+theme) + per-serieslegendShape@vx/legendAPI@vx/legendnew(vx-legend): add Line, legendLabelProps, more props in renderShape #749generics forI think types can be fixed in actual implementationDatum,XScaleInput, andYScaleInputwork correctly (partially done)integration withwill complete in actual implementation@vx/brush(partially done, not in demo)integration withwill complete in actual implementation@vx/zoom@techniq @hshoff @kristw