airbnb / visx

🐯 visx | visualization components
https://airbnb.io/visx
MIT License
19.49k stars 715 forks source link

[XYChart] <BarGroup> breaks when children are <React.Fragment>s #1040

Closed valtism closed 3 years ago

valtism commented 3 years ago

For reasons to do with accessing data from our redux store, I would like to have my <BarSeries> inside of a function component.

Unfortunately, the library will try to look at the < DataSetBar> component as the series it expects, then throw when the entry's data is undefined.

Is there any way to work around this?

Here is my code (abridged):

function DataGroupBar({ dataGroup }) {
  return (
    <AnimatedBarGroup>
      {dataGroup.map((dataSet) => (
        <DataSetBar
          key={dataSet.rowId}
          dataSet={dataSet}
        />
      ))}
    </AnimatedBarGroup>
  )
}

function DataSetBar({ dataSet }) {
  const { rowId, color } = dataSet
  const reportItem = useSelector((state) => state.reports[rowId])

  const actualsData = dateRange.map((month) => ({
      x: dateParser(month),
      y: computeBalance(reportItem),
    }))

  const budgetsData = dateRange.map((month) => ({
      x: dateParser(month),
      y: computeBalance(reportItem),
    }))

  return (
    <>
      <AnimatedBarSeries
        dataKey={`${reportItem.name} Actuals`}
        data={actualsData}
        {...accessors}
      />

      <AnimatedBarSeries
        dataKey={`${reportItem.name} Budgets`}
        data={budgetsData}
        {...accessors}
      />
    </>
  )
}

Right now, the only way I can see here is to move everything into my <DataGroupBar> component, forcing me to access all of state.reports in redux which loses performance as it causes a re-render every time any row in there changes, opposed to accessing just the single row.

I feel like the library should be able to tell that <DataSetBar> resolves into 2 <AnimatedBarSeries> and access them directly.

valtism commented 3 years ago

I think the core issue here is how <BarGroup> works. It isn't looking for a <BarSeries> in its children - it's looking for properties on the direct child. You can use a <div> if you want, as long as it has the dataKey and data properties on it.

I feel like it should be looking at it's children tree to find <BarSeries>, rather than just using the topmost element.

valtism commented 3 years ago

This is where the code breaks when hitting a fragment, or any element without a data property that is the direct ancestor of a <BarGroup>

https://github.com/airbnb/visx/blob/d7173660dd3b06e932407b60bcd82ffa98e68776/packages/visx-xychart/src/hooks/useScales.ts#L36

I would look into a fix, but the dataRegistry code seems complex.

williaster commented 3 years ago

Yeah this is because the <BarGroup /> expects its direct children to be <BarSeries />. Messing with / traversing children in React is a bit gross but seems like we should be able to create a helper function to traverse until a BarSeries component is found. Will try to look into this this week but don't have a ton of bandwidth.

williaster commented 3 years ago

FWIW, the data registry portion of the code shouldn't matter, it's this simple logic for children traversal pulling out the wrong children

valtism commented 3 years ago

Another issue here is that it would be nice to be able to have <BarGroup>s of <BarStack>s, which I don't think is currently possible with <XYChart>

tonyneel923 commented 3 years ago

I am running into this problem when dynamically generating bar series for a bar group and am not sure how to get around it.

Example code

<BarGroup padding={0}>
          {barChartsCollection.items.map((barChart) => {
            const vertical = xScale?.type === 'band';
            const { xAxis: currentXAxis, yAxis: currentYAxis } = barChart;
            return (
              <AnimatedBarSeries
                key={barChart.sys.id}
                dataKey={vertical ? currentYAxis : currentXAxis}
                data={items}
                xAccessor={(d: object) => vertical ? getLocValue(schema, d, currentXAxis) : getLocValue(schema, d, currentYAxis)}
                yAccessor={(d: object) => vertical ? getLocValue(schema, d, currentYAxis) : getLocValue(schema, d, currentXAxis)}
              />
            )
          })}
        </BarGroup>