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(xychart): add BarStack series #865

Merged
merged 2 commits into from
Oct 15, 2020
Merged

Conversation

williaster
Copy link
Collaborator

@williaster williaster commented Oct 9, 2020

TODO

🚀 Enhancements

This PR adds BarStack to @visx/xychart with an API that wraps BarSeries (this was planned in the POC, but could simplify in the future):

<BarStack>
  <BarSeries dataKey="sf" data={..} />
  <BarSeries dataKey="ny" data={..} />
</BarStack>

It also updates the /xychart demo

  • adds option to render BarStack (in the demo this is not compatible with LineSeries/BarSeries because they share dataKeys so controls are disabled accordingly)
  • adds option to render negative values. this lets me test that negative stacks work correctly (ditto for other Series types)

Tooltip

Negative values

Horizontal

Some notes on the implementation

  • The general flow is:

    1. collect props.data from child BarSeries
    2. create combined stacked data structure (using child props.dataKey)
    3. register data in the DataContext for each child dataKey, but update data + x/yAccessor to use the stacked data (so that scales are computed correctly)
    4. render BarStack using data + scales from DataContext
  • I first wrote this using @visx/shapes BarStack(Horizontal), but it doesn't allow us to use the stacked data independently of their scaled values (i.e., it creates the stack data structure and scales the stacked values). For the DataContext scales + findNearestDatum functions (TooltipContext), we need to add the stacked data itself as described in prev point

  • BarStacks implementation is currently is incompatible with Tooltip.snapTooltipToDatumX/Y.

    • This is tricky because Tooltip attempts to use DataContext for data + x/yAccessors to map the TooltipContext.tooltipData to the scaled positions. However tooltipData is typed as Datum, and the x/yAccessors + x/yScales from DataContext use the stacked SeriesDatum type. Will noodle on this more.

@kristw @techniq

@@ -17,20 +18,19 @@ type Props = {
height: number;
};

const xScaleConfig = { type: 'band', paddingInner: 0.3 } as const;
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 moved a lot of the Example params into ExampleControls

sharedTooltip,
showGridColumns,
showGridRows,
showHorizontalCrosshair,
showTooltip,
showVerticalCrosshair,
snapTooltipToDatumX,
snapTooltipToDatumY,
snapTooltipToDatumX: renderBarOrBarStack === 'bar' && snapTooltipToDatumX,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

moving stuff in here allows us to handle couple control logic like this

const entry = dataRegistry.get(barStack.key);

return entry
? barStack.map((bar, index) => {
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 mirrors the logic in BarStack/BarStackHorizontal

Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps do horizontal and other conditions outside the loop when possible.

const thickness = getBandwidth(horizontal ? yScale : xScale);
const halfThickness = thickness / 2;

let widthFn;
let heightFn;
let xFn;
let yFn;

if (horizontal) {
  widthFn = bar => (xScale(getSecondItem(bar)) || 0) - (xScale(getFirstItem(bar)) || 0);
  heightFn = () => thickness;
  xFn = bar => xScale(getFirstItem(bar));
  yFn = 'bandwidth' in yScale 
    ? bar => yScale(getStackValue(bar.data))
    : bar => Math.max((yScale(getStackValue(bar.data)) || 0) - halfThickness);
} else {
  widthFn = () => thickness;
  heightFn = bar => (yScale(getFirstItem(bar)) || 0) - (yScale(getSecondItem(bar)) || 0);
  xFn = 'bandwidth' in xScale
    ? bar => xScale(getStackValue(bar.data))
    : bar => Math.max((xScale(getStackValue(bar.data)) || 0) - halfThickness);
} 

return (
  <g className="visx-bar-stack">
    {stackedData.map((barStack, stackIndex) => {
      const entry = dataRegistry.get(barStack.key);
      if (!entry) return null;

      const y = yFn ?? (bar => yScale(entry.yAccessor(bar)));

      return barStack.map((bar, index) => {
        const barX = xFn(bar);
        if (!isValidNumber(barX)) return null;
        const barY = y(bar);
        if (!isValidNumber(barY)) return null;

        return (
          <rect
            key={`${stackIndex}-${barStack.key}-${index}`}
            x={barX ?? 0}
            y={barY ?? 0}
            width={widthFn(bar)}
            height={heightFn(bar)}
            fill={colorScale(barStack.key)}
            stroke="transparent"
          />
        );        
      });
    })}
  </g>
);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

great suggestion, also the special yFn handling made me realize I didn't actually need the entry accessor function 👍

@coveralls
Copy link

coveralls commented Oct 12, 2020

Pull Request Test Coverage Report for Build 207

  • 91 of 101 (90.1%) changed or added relevant lines in 9 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+1.1%) to 58.131%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/visx-xychart/test/mocks/getDataContext.ts 1 3 33.33%
packages/visx-xychart/src/components/series/BarStack.tsx 51 59 86.44%
Totals Coverage Status
Change from base Build 166: 1.1%
Covered Lines: 1378
Relevant Lines: 2280

💛 - Coveralls

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.

Mostly minor perf suggestion.

packages/visx-xychart/src/components/series/BarStack.tsx Outdated Show resolved Hide resolved
packages/visx-xychart/src/components/series/BarStack.tsx Outdated Show resolved Hide resolved
: Math.max((yScale(getStackValue(bar.data)) || 0) - barThickness / 2)
: yScale(entry.yAccessor(bar));

const barX: number | undefined = horizontal
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why does this one have type defined but barY does not?
Usually eslint doesn't like nested ternary?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

type was an oversight, I think we have nested ternary disabled in this repo because it's so often useful

const entry = dataRegistry.get(barStack.key);

return entry
? barStack.map((bar, index) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps do horizontal and other conditions outside the loop when possible.

const thickness = getBandwidth(horizontal ? yScale : xScale);
const halfThickness = thickness / 2;

let widthFn;
let heightFn;
let xFn;
let yFn;

if (horizontal) {
  widthFn = bar => (xScale(getSecondItem(bar)) || 0) - (xScale(getFirstItem(bar)) || 0);
  heightFn = () => thickness;
  xFn = bar => xScale(getFirstItem(bar));
  yFn = 'bandwidth' in yScale 
    ? bar => yScale(getStackValue(bar.data))
    : bar => Math.max((yScale(getStackValue(bar.data)) || 0) - halfThickness);
} else {
  widthFn = () => thickness;
  heightFn = bar => (yScale(getFirstItem(bar)) || 0) - (yScale(getSecondItem(bar)) || 0);
  xFn = 'bandwidth' in xScale
    ? bar => xScale(getStackValue(bar.data))
    : bar => Math.max((xScale(getStackValue(bar.data)) || 0) - halfThickness);
} 

return (
  <g className="visx-bar-stack">
    {stackedData.map((barStack, stackIndex) => {
      const entry = dataRegistry.get(barStack.key);
      if (!entry) return null;

      const y = yFn ?? (bar => yScale(entry.yAccessor(bar)));

      return barStack.map((bar, index) => {
        const barX = xFn(bar);
        if (!isValidNumber(barX)) return null;
        const barY = y(bar);
        if (!isValidNumber(barY)) return null;

        return (
          <rect
            key={`${stackIndex}-${barStack.key}-${index}`}
            x={barX ?? 0}
            y={barY ?? 0}
            width={widthFn(bar)}
            height={heightFn(bar)}
            fill={colorScale(barStack.key)}
            stroke="transparent"
          />
        );        
      });
    })}
  </g>
);

packages/visx-xychart/src/utils/getBarStackRegistryData.ts Outdated Show resolved Hide resolved
packages/visx-xychart/src/utils/getBarStackRegistryData.ts Outdated Show resolved Hide resolved
new(demo/xychart): add BarStack

new(xychart): add horizontal support to BarStack

new(demo/xychart): add negative values, refactor accessors

internal(xychart/barstack): refactor to use d3-shape :/

new(xychart): clean up BarStack, add mouse events

fix(demo/xychart): disable tooltip snapping when using BarStack

internal(xychart/BarStack): add util comments, simplify combineBarStackData

types(xychart): fix some registry types

internal(xychart/BarStack): make perf improvements
* tes(xychart/BarStack): add tests

* test(xychart/combineBarStackData): add tests

* test(xychart/findNearestStackDatum): add tests

* internal(xychart): remove console logs in tests
@williaster williaster merged commit bcae42d into master Oct 15, 2020
@williaster williaster deleted the chris--xychart-stackedbar branch October 15, 2020 23:20
@williaster williaster added this to the 1.1.0 milestone Oct 17, 2020
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.

3 participants