Brush Interaction for XY-chart #117

conglei · 2018-08-13T07:58:36Z

💔 Breaking Changes

🏆 Enhancements

Brush interaction is supported!

📜 Documentation

🐛 Bug Fix

🏠 Internal

williaster

Looking great overall! 🙌

williaster · 2018-08-13T17:13:11Z

packages/xy-chart/src/index.js

@@ -25,3 +25,4 @@ export { PatternLines, PatternCircles, PatternWaves, PatternHexagons } from '@vx
 export { withScreenSize, withParentSize, ParentSize } from '@vx/responsive';
 export { default as withTheme } from './enhancer/withTheme';
 export { chartTheme as theme } from '@data-ui/theme';
+export { default as Brush } from './selection/Brush';


do you think all of the brush components should go under src/components/brush/... and the utils could go under src/util/brush?

So here, I was thinking for a long term, will we keep the code here or we should move it to vx? The reason I put it in the utils is that I think now it is the temporary solution.

williaster · 2018-08-13T17:18:16Z

packages/xy-chart/src/chart/XYChart.jsx

+                return React.cloneElement(Child, {
+                  xScale,
+                  yScale,
+                  innerHeight,


curious if innerWidth/Height can be computed from the passed scales?

williaster · 2018-08-13T17:19:29Z

packages/xy-chart/src/utils/brush/Brush.js

+    }));
+  }
+
+  width() {


nit -- I'd call these getWidth/Height for clarity

williaster · 2018-08-13T17:20:22Z

packages/xy-chart/src/utils/brush/Brush.js

+          }}
+        </Drag>
+        {/* selection */}
+        {start &&


if start/end happened to be 0 these might not work

williaster · 2018-08-13T17:21:31Z

packages/xy-chart/src/utils/brush/Brush.js

+              stageWidth={stageWidth}
+              stageHeight={stageHeight}
+              brush={{ ...this.state }}
+              fill="rgba(4, 154, 251, 1.000)"


could we expose style parameters as props do you think?

williaster · 2018-08-13T17:22:16Z

packages/xy-chart/src/utils/brush/Brush.js

+                stageWidth={stageWidth}
+                stageHeight={stageHeight}
+                updateBrush={this.update}
+                brush={this.state}


any reason you pass state directly here vs copy it for BrushSelection?

williaster · 2018-08-13T17:25:35Z

packages/xy-chart/src/utils/brush/RightHandle.js

+import React from 'react';
+import { Drag } from '../drag';
+
+function handleResizeMove(drag, onBrushEnd) {


I'd possibly add these as statics

williaster · 2018-08-13T17:26:49Z

packages/xy-chart/src/utils/drag/Drag.js

@@ -0,0 +1,115 @@
+import React from 'react';


I think you'll get some linting errors for Drag.js not having a .jsx name. Also similar to brush we could move this to a src/components folder.

conglei · 2018-08-21T20:59:22Z

@williaster Now the code is done for the first version. PTLA :)

williaster

This is amazing! really nice work!!! 🤑

Sorry for the many comments, they're mostly just conventions/nothing huge. I appreciate all of the time you've put into this!

williaster · 2018-08-21T22:45:05Z

packages/demo/examples/01-xy-chart/BrushableLineChart.jsx

+          xScale={{ type: 'time' }}
+          yScale={{ type: 'linear' }}
+          margin={{ left: 100, top: 64, bottom: 64 }}
+          {...rest}


I think you can get rid of this right?

williaster · 2018-08-21T22:45:50Z

packages/demo/examples/01-xy-chart/BrushableLineChart.jsx

+          margin={{ left: 100, top: 64, bottom: 64 }}
+          {...rest}
+        >
+          <LinearGradient id="area_gradient" from={colors.categories[2]} to="#fff" />


do you need the gradient + pattern? (I'd just remove anything unneeded in case people look at the example usage)

williaster · 2018-08-21T22:53:13Z

packages/xy-chart/src/chart/XYChart.jsx

@@ -259,6 +275,10 @@ class XYChart extends React.PureComponent {
    const { numXTicks, numYTicks } = this.getNumTicks(innerWidth, innerHeight);
    const barWidth = xScale.barWidth || (xScale.bandwidth && xScale.bandwidth()) || 0;
    const CrossHairs = []; // ensure these are the top-most layer
+    let hasBrush = false;


could you initially set let Brush = null and then just call Brush && React.cloneElement(Brush ...) below to get rid of the extra hasBrush var?

williaster · 2018-08-21T22:55:48Z

packages/xy-chart/src/chart/XYChart.jsx

+                innerHeight,
+                innerWidth,
+                margin,
+                registerStartEvent:


could you add a comment about why this isn't needed for a series event trigger?

Good catch! In deed for area chart and bar chart, we do need to do like this. Will revise the code logic.

williaster · 2018-08-21T23:00:48Z

packages/xy-chart/src/selection/Brush.jsx

+    } else if (brushRegion === 'yAxis') {
+      top = 0;
+      brushRegionHeight = innerHeight;
+      if (yAxisOrientation === 'right') {


does anything break if there are no axes passed to XYChart and brushRegion is the axis that's not passed? (which would make x/yAxisOrientation null)?

I guess that makes no sense from a user perspective, but maybe should fail intelligently?

@williaster it won't to broken. User can still brush from margin areas.

williaster · 2018-08-21T23:11:53Z

packages/xy-chart/src/utils/brush/RightHandle.jsx

+          <rect
+            width={6}
+            height={height}
+            x={brush.domain.x1 - left - 3}


is this getting a magic-number error?

williaster · 2018-08-21T23:13:12Z

packages/xy-chart/src/utils/brush/RightHandle.jsx

+              cursor: brush.isBrushing ? undefined : 'ew-resize',
+            }}
+            onMouseUp={event => {
+              if (brush.isBrushing) {


I'd vote for moving these into class functions/binding them, so they aren't created on renders

williaster · 2018-08-21T23:14:52Z

packages/xy-chart/src/utils/drag/Drag.jsx

@@ -0,0 +1,119 @@
+/* eslint react/jsx-handler-names: 0 */


could we also rename them according to the eslint conventions?

williaster · 2018-08-21T23:15:16Z

packages/xy-chart/src/utils/drag/Drag.jsx

+  }
+}
+
+Drag.propTypes = {


same comment re top of file for Airbnb style convention.

williaster · 2018-08-21T23:16:41Z

packages/xy-chart/test/selection/Brush.test.jsx

+    );
+  }
+
+  function simulateFullDrag(target) {


conglei · 2018-08-22T03:00:01Z

@williaster I've revised the code per comments :) Thanks for catching so many issues :)

For react/jsx-handler-names, as we discussed, all the files with this will be moved to @vx/brush and the current naming convention follows the style in @vx

conglei · 2018-08-22T03:01:58Z

Hmm, I still haven't figured out why the testings failed here. :(

williaster · 2018-08-22T07:03:33Z

Hmm, I'll try to pull/debug tomorrow, and worse case scenario we can merge to get into DLS. Thanks for making the other fixes!

williaster · 2018-08-22T17:24:09Z

I noticed a couple of possible issues when playing with it:

In this one I'm not able to move the brush all the way to the right, and at the end it jumps vertically

In this one I'm able to move the x-axis brush vertically even though that should be disabled.

conglei · 2018-08-22T18:09:12Z

@williaster it seems to be resizing issue. so the bugs happened after the window size changed right ?

conglei · 2018-08-22T19:02:50Z

@williaster Just fixed the issue. Maybe we can first merge this and I will create an issue for remaining tasks on the brush package.

williaster · 2018-08-22T19:04:01Z

SGTM 👍 Thanks for all of the time spent so far 🙌Excited for this! 🎉

working version

97b6f2c

conglei mentioned this pull request Aug 13, 2018

any plan to support brush? #106

Closed

williaster reviewed Aug 13, 2018

View reviewed changes

Conglei Shi added 5 commits August 20, 2018 09:41

enable brush together with container level interation

95108b4

fixed lints

5caf624

added onBrushEnd and DisableDraggingSelection

420090f

fixed typos

6e02d6f

fixed more lints and complete the logic for onBrushEnd

3b597a7

Conglei Shi added 3 commits August 21, 2018 14:00

removd AreaDifferenceSeriesExampleWithBrush

d139626

added tests and revised the code to improve testability

5421253

refactored the code

375de16

williaster reviewed Aug 21, 2018

View reviewed changes

enhanced propTypes and fixed triggering issues

061752c

tried to fix travis

c99ba69

fixed the issue that the bounds are not updated after window resizing

34de8ea

williaster merged commit f7d35b9 into williaster:master Aug 22, 2018

williaster changed the title ~~[WIP] Brush Interaction for XY-chart~~ Brush Interaction for XY-chart Aug 22, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Brush Interaction for XY-chart #117

Brush Interaction for XY-chart #117

conglei commented Aug 13, 2018 •

edited

Loading

williaster left a comment

williaster Aug 13, 2018

conglei Aug 21, 2018

williaster Aug 13, 2018

williaster Aug 13, 2018

williaster Aug 13, 2018

williaster Aug 13, 2018

williaster Aug 13, 2018

williaster Aug 13, 2018

williaster Aug 13, 2018

conglei commented Aug 21, 2018

williaster left a comment

williaster Aug 21, 2018

williaster Aug 21, 2018

williaster Aug 21, 2018

williaster Aug 21, 2018

conglei Aug 21, 2018

williaster Aug 21, 2018

conglei Aug 22, 2018

williaster Aug 21, 2018

williaster Aug 21, 2018

williaster Aug 21, 2018

williaster Aug 21, 2018

williaster Aug 21, 2018

conglei commented Aug 22, 2018

conglei commented Aug 22, 2018

williaster commented Aug 22, 2018

williaster commented Aug 22, 2018

conglei commented Aug 22, 2018

conglei commented Aug 22, 2018

williaster commented Aug 22, 2018

Brush Interaction for XY-chart #117

Brush Interaction for XY-chart #117

Conversation

conglei commented Aug 13, 2018 • edited Loading

williaster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

conglei commented Aug 21, 2018

williaster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

conglei commented Aug 22, 2018

conglei commented Aug 22, 2018

williaster commented Aug 22, 2018

williaster commented Aug 22, 2018

conglei commented Aug 22, 2018

conglei commented Aug 22, 2018

williaster commented Aug 22, 2018

conglei commented Aug 13, 2018 •

edited

Loading