-
Notifications
You must be signed in to change notification settings - Fork 720
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
RFC: Proposal for new xy-chart
package
#734
Comments
First, excellent writeup! You concisely and completely covered very well. Question, in the Proposed API example, the I figure we'll want to start working on a PoC (maybe in codesandbox or something)? |
Another question, what about Hierarchical layouts (Treemap, Partition (Sunburst, Icicle), Tree, Pack, Sankey, etc). I assume instead of |
Good questions. I propose this RFC should limit the scope to |
@kristw Agreed, and if the plan is to handle it as a separate component (HierarchyChart/etc) we can do that in the future. |
Yes! My mistake, I've updated it to
Also agree, I think a POC sandbox should be easy to create from the
Love the idea of a |
Features
All the good stuffs! Comment-1
I would like to propose one change from the original Full disclosure: I have been developing the Comment-2
Here are a few additional alternatives. I supposed the Option 3a. The data have to be passed twice, with the provided coding convention below.const dataset = {
cost: [{ x: 1, y: 2, datum: ... }, { x: 3, y: 4, datum: ... }],
revenue: [{ x: 1, y: 2, datum: ... }, { x: 3, y: 4, datum: ... }],
};
return (
<XYChart dataset={dataset} >
<LineSeries key="cost" data={dataset[cost]} />
<LineSeries key="revenue" data={dataset[revenue]} />
</XYChart>
); Option 3b. The data is passed once to the chart, with series referring to its
|
Comment-3Now the type Props = {
...,
computeDomain: ({ dataset, xScale, yScale }) => void;
}
<XYChart
dataset={...}
xScale={...}
yScale={...}
computeDomain={({ dataset, xScale, yScale }) => {
// computes and sets xScale.domain(...)
// computes and sets yScale.domain(...)
}}
/> |
Hmm, to me this adds a lot of eng effort + a learning curve which detracts a bit from the goal of "easy to use out of the box". If you pass in a I think the scale config in
|
I am not sure which one is easier to learn. If import { scaleLinear } from 'd3-scale';
<XYChart xScale={scaleLinear().domain([0, 100]).nice(true)} /> vs. learning another construct which is specific to <XYChart xScale={{ type: 'linear', domain: [0, 100], nice: true }} /> I am not saying we strongly remove this, but perhaps it can be opt-in. import { scaleLinear } from 'vx-scale';
<XYChart xScale={scaleLinear({ domain: [0, 100], nice: true })} />
This is related to the above proposal if the Example scenarios:
Arguably for example 1-3, the domain could be computed beforehand and passed into This is probably an advanced feature tho so usually new developers won't touch. Can drop this part from the discussion for v1 scope.
There are possibilities. The part that make it a bit tangle is ability to lookup color scheme by names in |
Regarding custom scales with percentages, we do this in our apps as well, either as 0-100%... we also have a There are also some primatives we have such as interface OverlayBarProps {
x: number;
xScale: ScaleBand<any>;
yScale: ScaleLinear<any, any>;
}
function OverlayBandBar(props: OverlayBarProps) {
const { x, xScale, yScale } = props;
return (
<Bar
x={x - (xScale.padding() * xScale.step()) / 2}
y={0}
width={xScale.step()}
height={yScale.range()[0]}
fill="rgba(0,0,0,.1)"
/>
);
} We also have |
^awesome examples @techniq 😍 I agree we should support all of these, it should be straight forward with I'll open a PR soon for ease of commenting. I'm trying to work out more of the details for events. Some initial notes: Mapping events <> Datum
General user feedback (from non-vis engineers) was that the 3 options were confusing and they didn't understand when to use what 😢 So I'm thinking about how we could improve the dev experience by doing this automatically. I think generally we could
Event contextIn thinking more about perf, since any child who uses a given context updates when a context value changes, I think it might be preferable to have a separate context for events, which |
Regarding tooltips and bar charts, one thing I do is render a For example on the our const VerticalBarShape: React.FunctionComponent<VerticalBarShapeProps> = ({
bar,
index,
xScale,
yScale,
color,
barProps,
barTargetProps,
showTooltip,
hideTooltip
}) => {
const [yMax] = yScale.range();
return (
<React.Fragment key={`bar-${bar.x}`}>
<Bar
x={bar.x}
y={bar.y}
width={bar.width}
height={bar.height}
fill={color || bar.color || blue[500]}
{...(barProps && barProps({ bar, index }))}
/>
<BarTarget
bar={bar}
x={bar.x - (xScale.padding() * xScale.step()) / 2}
y={0}
width={xScale.step()}
height={yMax}
showTooltip={showTooltip}
hideTooltip={hideTooltip}
// fill="rgba(255,0,0,.1)"
{...(barTargetProps && barTargetProps({ bar, index }))}
/>
</React.Fragment>
);
}; we also have a const HorizontalBarShape: React.FunctionComponent<HorizontalBarShapeProps> = ({
bar,
index,
xScale,
yScale,
color,
barProps,
barTargetProps,
showTooltip,
hideTooltip
}) => {
const [xMin, xMax] = xScale.range();
return (
<React.Fragment key={`bar-${bar.x}`}>
<Bar
x={bar.x}
y={bar.y}
width={bar.width}
height={bar.height}
fill={color || bar.color || blue[500]}
{...(barProps && barProps({ bar, index }))}
/>
<BarTarget
bar={bar}
x={0}
y={bar.y - (yScale.padding() * yScale.step()) / 2}
width={xMax}
height={yScale.step()}
showTooltip={showTooltip}
hideTooltip={hideTooltip}
{...(barTargetProps && barTargetProps({ bar, index }))}
/>
</React.Fragment>
);
}; lastly, sometimes we override the "full band" targets where it makes sense, for example on a TimelineChart (Gnatt chart). It this case we just wante the target to be the size of the bar (plus the circles on the sides which gives it a "pill" shape. <Fragment key={`${y(d)}-${i}`}>
<HorizontalBarShape
bar={bar}
index={i}
xScale={xScale}
yScale={yScale}
color={color ? color(d, i) : undefined}
barProps={barProps}
barTargetProps={({ bar, index }) => ({
x: bar.x - pointRadius,
width: bar.width + pointRadius * 2,
...(barTargetProps && barTargetProps({ bar, index }))
})}
showTooltip={showTooltip}
hideTooltip={hideTooltip}
/>
{endpoints && (
<>
<circle
key={'start-' + start.toString()}
fill={color ? color(d, i) : undefined}
cx={xScale(start)}
cy={yScale(y(d)) + pointRadius}
r={pointRadius}
style={{ pointerEvents: 'none' }}
{...(endpointProps &&
endpointProps({ point: start, index: 0, bar }))}
/>
<circle
key={'end-' + end.toString()}
fill={color ? color(d, i) : undefined}
cx={xScale(end)}
cy={yScale(y(d)) + pointRadius}
r={pointRadius}
style={{ pointerEvents: 'none' }}
{...(endpointProps &&
endpointProps({ point: end, index: 1, bar }))}
/>
</>
)}
</Fragment> I'm not sure how helpful these snippets are but sadly I don't have an example in codesandbox right now. Regarding the separate event context, that seems like a good idea for performance reason (unless we can do a lot of memoization / etc, but taking away as many foot guns as possible is always good. |
Wanted to get resolution on a couple of high-level directions while I continue on the POC: Resolution on
|
RE: Resolution on data at the Chart container level versus the DataSeries level
I feel less strongly about this one and can go either way. My main concern was the challenges for library maintainer to handle the performance optimization, but can see why it can be more developer-friendly. (And since the maintainer is willing to handle the complexity, I will not discourage. 😁 ) Notes:
RE: Resolution on scales vs scale config
Agree with making it prioritized for react-oriented developers, but also would like to have support for advanced use cases and/or open the door for d3-oriented developers. We should be able to do the best of both worlds? I personally ran into the situation that I already have a D3 scale and had to choose between (a) convert my D3 scale to the config, only for it to be reconstructed as a D3 scale again with subset of the behavior I had in my original scale (b) not using
Let me clarify a bit. My original proposal is NOT getting rid of the config, but extends the Option A: The current behaviortype XScale = {
type: 'linear' | 'band' | 'time' | ... ;
domain?: ScaleInput[];
range?: ScaleOutput[];
rangeRound?: ScaleOutput[];
nice?: boolean;
clamp?: boolean;
}
<XYChart xScale={{ type: 'linear', ... }} /> Cons: I can't use my own D3 scale and workarounds are not satisfying. Option B: Also allow D3 scales as inputimport {
ScaleLinear,
ScaleLogarithmic,
ScalePower,
ScaleTime,
ScalePoint,
ScaleBand,
} from 'd3-scale';
type SupportedD3Scale =
| ScaleLinear
| ScaleLogarithmic
| ScalePower
| ScaleTime
| ScalePoint
| ScaleBand
type XScale = {
type: 'linear' | 'band' | 'time' | ... ;
domain?: ScaleInput[];
range?: ScaleOutput[];
rangeRound?: ScaleOutput[];
nice?: boolean;
clamp?: boolean;
} | SupportedD3Scale;
// You can pass config
<XYChart xScale={{ type: 'linear', ... }} />
// or You can pass scale.
<XYChart xScale={scaleLinear()} /> Pros: No breaking change. Cons: Unclear if the input scale should update domain from the data. Needs a way to specify that. Option C: Only allow D3 scales as inputThis is a slight variation of the Option B. // when using config
import { createScale } from '@vx/scale')
<XYChart xScale={createScale({ type: 'linear' })} />
// when using scale
import { scaleLinear } from 'd3-scale')
<XYChart xScale={scaleLinear()} /> Pros: uniform input for the Cons:
Option D: Let this be another typetype XScale = {
type: 'linear' | 'band' | 'time' | ... ;
domain?: ScaleInput[];
range?: ScaleOutput[];
rangeRound?: ScaleOutput[];
nice?: boolean;
clamp?: boolean;
} | {
type: 'custom',
scale: SupportedD3Scale;
updateDomain: boolean | (scale, domainFromData) => SupportedD3Scale,
updateRange: boolean | (scale, dimension) => SupportedD3Scale,
}
<XYChart xScale={{
type: 'custom'
scale: myOwnScale,
updateDomain: false,
updateRange: true
}} /> I think this looks safe. The My preference at the moments is D, followed by B with a few more additional props to handle domain/range updates but that will double the number of props because RE: Other smaller pointsChecking if an input is a scale config or D3 scale.
Can check if @vx/scale config can do everything a D3 scale does
This requires If you ensure 100% parity and the developer should send PRs/wait for next release, that signals a forever contract for the maintenance of Making @vx/scale better// this is always true
type ScaleOutput = number[];
// in reality we probably need to make this a union of different configs per type
interface ScaleConfig<ScaleInput> {
type: 'linear' | 'band' | 'time' | ... ;
domain?: ScaleInput[];
range?: ScaleOutput[];
rangeRound?: ScaleOutput[];
nice?: boolean;
clamp?: boolean;
} I have written all of these already in I can take over |
Something else to consider about scales is animating. I've been experimenting with this again (mostly with hierarchy charts but also zooming scales for time series, etc) you can see here: Currently I interpolate/tween the values using d3-interpolate and call scale.domain(...).range(...) in the Spring I haven't checked in some of my recent changes (working on a zoomable Treemap and adding other hierarchy layouts, still just experimenting though). I'm hoping to uncover a good pattern / custom animated scale hook for this at some point. |
I just had a chance to look at the vx-brush example as I thought it would be similar to animation, although it looks like you build a new scale instance (utilizing |
I just pulled out an AnimatedScale that better shows this (and cleans up some duplication). I'm not fully settled on the API yet (still experimenting and interpolating with multiple scales (transforms, etc) has some tearing when done individually it appears. |
@kristw I think it'd be awesome to improve the typings in @techniq thanks for highlighting more of the complexities with animations and scale updates, I agree we should flesh that out more. your approach is really interesting, that seems like a pretty good API to me overall (we should try to update the brush example to use that approach, we're definitely just brute-forcing it right now 😬 ). A problem I've not known how to solve with updating Separately, it'd be sweet to add patterns/things like |
@williaster With I know the react-spring organization also has their own state management call zustand and one of their features is I also just read up a little more as I was curious how zustand works with Concurrent React. It sounds like they are still debating but there was some promise with a new hook added to React (useMutableSource) although it sounds like they are still having issues with it. We might be able to use this hook directly though. Here's a comparison of different state solutions and if they will work with Concurrent Mode. I'm not using Concurrent Mode yet myself, but thought we'd want to plan for it (and at least not pin ourselves in a corner). Regarding memoizing, I've been waiting to mention this, but it seems like we should memoize all our hierarchy layouts such as Treemap, Partition, etc. I think this is were the slight delay in my examples is coming from after clicking on a child (it's recalculating the layout on each render). As that point we could also expose them as hooks |
There is also react-tracked that sounds promosing. It also looks to have been ported to useMutableSource. |
I'm familiar with the use of
nice, hadn't seen this one. They note
I wonder if this is actually relevant for us. If we have
I hadn't seen this either, this definitely seems like a good avenue to go for concurrent mode. I've played around with it once and it was a mess so haven't returned yet. If we do go with context, it seems like this wouldn't be necessary? But definitely good to think about future-proof wise and not cornering ourselves as you say.
LOVE THIS IDEA ❤️ I have a bit more time today so will get the PR up for the POC, and try to play with animation. |
Okay finally! (sorry there were some problems with other Here's the PR #745 🎉 Have a lot of things working (love the dark mode + animation!), really curious about your feedback on the |
Going to close this issue as implementation is underway. You can follow progress in the project tracker. |
Motivation
vx
packages are (by design) low-level and modular which maximizes their flexibility, but also requires more work to create even simple charts. My library@data-ui/xy-chart
is built on top ofvx
and was designed to make it easier to create common charts with less work. To solve this use-case within thevx
ecosystem, and consolidate these efforts,@data-ui/xy-chart
is being deprecated and we plan to port its functionality to a newvx
package@vx/xy-chart
.Goals of this RFC
XYChart
APIFeatures
To have feature parity with
@data-ui/xychart
, the new package should support the following:x-
andy- scales
across all data seriesXYChart
across all data series*Series
which are mostly wrappers aroundvx
Bar
,Line
,Point
,Area
,AreaDifference
,StackedArea
,StackedBar
,GroupedBar
,BoxPlot
,ViolinPlot
,Interval
CrossHair
component for use withTooltip
XAxis
andYAxis
components fromvx
horizontal
andvertical
reference lines
x-
andy-gridlines
brush
functionality (pre@vx/brush
)LinearGradient
,Patterns
, and a chart theme viaprops
FocusBlur
handlers that area11y
accessible / tab-ableWe would also like to add additional support for the following:
New near-term features
TypeScript
datum
shape +x
/y
data accessors (currently requires{ x, y }
datum
shape)hooks
supportNew mid-term features
Tooltip
in aPortal
to fix z-index stacking context problemLegend
s@vx
primitives likebrush
,zoom
, anddrag
canvas
support –vx
is currently mostlysvg
based (this likely requires updates in othervx
packages)@data-ui
does not support animation. while this may not need to be fully baked in we should expose hooks to animatexy-chart
API
@techniq has done some great research on declarative
react
chart APIs here. Generally they are composable:However there are some key differences:
data
provided at theDataSeries
level vs thechart
container levelDataSeries
level – ✅ favoredPros 👍
data
to the series that will visually represent it@vx/shape
's (the basis forDataSeries
) currently require data, so this is more consistent with separate package APIsx-
andy-
extent from theirdata
, and theChart
container can simply collect thesehorizontal
orientation to be pushed to theSeries
-level without requiring it to be implemented by allSeries
and theChart
container doesn't need to have any knowledge of itCons 👎
Chart
container needs a way to access the data across all seriesChart
container level – ❌ disfavoredPros 👍
Chart
needs access to all data in order to providex-
andy-
scales; this makes that easy.Cons 👎
Series
may require custom logic to computex-
andy-
extent from theirdata
(e.g., a bar stack) which theChart
needs to be aware of in this modelkey
accessors at theSeries
-level forSeries
dataDataSeries
to have the same data length (or be filled with empty values)Mouse and Touch events
Mouse and touch events are handled in several ways
Standard mouse events – ✅ favored
react-vis
and@data-ui
expose mouse and touch events at both theChart
andSeries
level; these use standardreact
events such asonClick
,onTouchMove
, etc.Custom event syntax – ❌ disfavored
Some libraries like
Victory
have their own custom event system with non-standard syntax and selection language.react
hooksI've not been able to find any
react
vis libraries which exposehooks
. Feels like an opportunity on top of a render / component API 😏Implementation
We'd like to improve upon the following limitations of
@data-ui
v1
implementation:Written in TypeScript
@data-ui
was written in JavaScript, butvx
is now a TypeScript project and typings will be similarly useful for@data-ui
.Use
react
context
over cloning children@data-ui
was implemented before the new / more robust16.3
context
API. Therefore chartstyles
and sharedscales
are passed viaprop
s +React.cloneElement
. Combined withhooks
, usingcontext
should open up a whole new set of API possibilities (see below).What is kept in context
The function of the
XYChart
wrapper largely equates to managing shared state across the elements of a chart, which components can leverage as needed. . This includestheme
+styles
xScale
that accounts for data range of all chart series and chartwidth
+margin
yScale
that accounts for data range of all chart series and chartheight
+margin
tooltipData
+tooltipCoords
, when applicableIn addition to chart
width
+height
, theChart
container must have knowledge of alldata
(+annotation
) values to computescale
s appropriately. Rather than having theChart
introspect props fromchild
DataSeries
orAnnotations
(which can get gnarly) we propose thatDataSeries
andAnnotations
register theirdata
,xValues
, andyValues
incontext
.This pushes the logic of
data <> x/y extent
mapping toDataSeries
rather than theChart
, and allows theChart
to leverage these values to compute scales properly. It could look something likeUnknowns
I'm unsure if there are major performance implications of using hooks ⚡ 🐌
Proposed API
The same functionality could be provided in a
component
API:cc @hshoff @techniq @kristw
The text was updated successfully, but these errors were encountered: