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.

Sign up for GitHub

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

Create ZoomHandler #1354

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Create ZoomHandler #1354

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1ee7dee
first attempt to fix highlighting
Jun 6, 2020
61a3c9d
Create ZoomHandler
Jun 10, 2020
e4bb21c
removed 'useStateWithGet'
Jun 10, 2020
e51407b
add tests
Jun 11, 2020
c7332a0
Allow using Storybook for local development.
Jun 6, 2020
10da92e
Added Window
Jun 13, 2020
2932243
fix test
Jun 13, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 14 additions & 6 deletions .eslintrc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
"extends": [
"eslint:recommended",
"plugin:react/recommended",
"plugin:react-hooks/recommended",
"plugin:jest/recommended",
"prettier",
"prettier/react"
Expand All @@ -19,8 +20,8 @@
"**/dist/",
"**/es/"
],
"settings":{
"react":{
"settings": {
"react": {
"version": "detect"
}
},
Expand All @@ -31,12 +32,19 @@
},
"rules": {
"consistent-return": 0,
"max-len": [1, 110, 4],
"max-params": ["error", 6],
"max-len": [
1,
110,
4
],
"max-params": [
"error",
6
],
"object-curly-spacing": 0,
"babel/object-curly-spacing": 2,
"jest/require-top-level-describe":"error",
"jest/require-top-level-describe": "error",
"react/prop-types": "off",
"prettier/prettier": "warn"
}
}
}
1 change: 1 addition & 0 deletions packages/react-vis/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@
"enzyme-adapter-react-16": "^1.15.2",
"enzyme-to-json": "^3.5.0",
"eslint-plugin-jest": "^23.13.2",
"eslint-plugin-react-hooks": "^4.0.4",
"jest": "^25.5.4",
"jsdom": "^9.9.1",
"node-sass": "^4.9.3",
Expand Down
2 changes: 2 additions & 0 deletions packages/react-vis/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,8 @@ export Treemap from 'treemap';

export ContentClipPath from './plot/content-clip-path';

export ZoomHandler from './plot/zoom-handler';

export {
makeHeightFlexible,
makeVisFlexible,
Expand Down
19 changes: 17 additions & 2 deletions packages/react-vis/src/plot/xy-plot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@ import {

import CanvasWrapper from './series/canvas-wrapper';

import {Event} from '../utils/events';

const ATTRIBUTES = [
'x',
'y',
Expand Down Expand Up @@ -140,7 +142,14 @@ class XYPlot extends React.Component {
const data = getStackedData(children, stackBy);
this.state = {
scaleMixins: this._getScaleMixins(data, props),
data
data,
events: {
mouseMove: new Event('move'),
mouseDown: new Event('down'),
mouseUp: new Event('up'),
mouseLeave: new Event('leave'),
mouseEnter: new Event('enter')
}
};
}

Expand Down Expand Up @@ -222,7 +231,8 @@ class XYPlot extends React.Component {
...scaleMixins,
...child.props,
...XYPlotValues[index],
...dataProps
...dataProps,
events: this.state.events
});
});
}
Expand Down Expand Up @@ -348,6 +358,7 @@ class XYPlot extends React.Component {
component.onParentMouseDown(event);
}
});
this.state.events.mouseDown.fire(event);
};

/**
Expand All @@ -367,6 +378,7 @@ class XYPlot extends React.Component {
component.onParentMouseEnter(event);
}
});
this.state.events.mouseEnter.fire(event);
};

/**
Expand All @@ -386,6 +398,7 @@ class XYPlot extends React.Component {
component.onParentMouseLeave(event);
}
});
this.state.events.mouseLeave.fire(event);
};

/**
Expand All @@ -405,6 +418,7 @@ class XYPlot extends React.Component {
component.onParentMouseMove(event);
}
});
this.state.events.mouseMove.fire(event);
};

/**
Expand All @@ -424,6 +438,7 @@ class XYPlot extends React.Component {
component.onParentMouseUp(event);
}
});
this.state.events.mouseUp.fire(event);
};

/**
Expand Down
175 changes: 175 additions & 0 deletions packages/react-vis/src/plot/zoom-handler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,175 @@
import React, {useEffect, useState, useCallback, useRef} from 'react';
import {getCombinedClassName} from '../utils/styling-utils';
import {getAttributeScale} from '../utils/scales-utils';

const DEFAULT_STATE = {
brushing: false,
bounds: null,
startPosition: null
};

const useStateWithGet = initialState => {
const [state, setState] = useState(initialState);
const ref = useRef();
ref.current = state;
const get = useCallback(() => ref.current, []);
return [state, setState, get];
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will move this to a shared location, however I left it here for now to give it some better visibility.

This acts just like useState however it returns a third parameter which is a function that returns the value.
The function is memoized so there will only be one instance per component, however when called, it will return the current state.

This makes it ideal when the handlers, such as useEffect and useCallback need the current value, however it doesn't change the identity of handler.

Using the getState in the useCallbacks below drastically reduces the number of subscribe/unsubscribe calls, as well as the number of re-renders. Without this, the callback for onMouseUp would be invalidated on every mouse move (while dragging)

Copy link
Contributor

@reitlepax reitlepax Jun 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I am not very comfortable with this, if I understand correctly it looks like a hack to get around the dependencies of useCallback and useEffect and this could lead to a bug.

I am not talking about this specific case because I am not sure, but it could lead to situations where the state value is changed, but since getState is used as dependency for a useEffect , it won't be called with the new value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, this is an approach to get around the dependencies and it could be abused which may cause bugs.

The main bug that I can see arising only really applies to useMemo.
If the getState is passed into the dependencies of useMemo a change of state will not cause the useMemo handler to re-fire, and therefore the result would be based off of the previous value.

With useCallback and useEffect, you won't really have these problems since they would always use the latest values, and their execution is not dependant on the render function.

One other way to accomplish this would be

const onMouseUp = useCallback(() => {
  setState(state => {
     if (size > 5) {   // shorted for brevity
        onZoom(state.bounds);
    }
   setState(DEFAULT_STATE);
}, [])

This example here, I find is much worse as the function passed to setState is no longer pure.
Here is an issue discussing other ways around it as well. facebook/react#14543

Copy link
Contributor Author

@Xiot Xiot Jun 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Another, more standard way of doing this, would be as follows:

const [state, setState] = useState(DEFAULT_STATE);
const stateRef = useRef(state);

const onMouseUp = useCallback(() => {
   const state = stateRef.current;
   ...
}, [])

In the end this is the same as the custom hook, however this is more explicit.
And as such, it could be commented in-line to better explain why the functionality is like that.

This would also eliminate the temptation to use this in places where it would cause bugs.

Copy link
Contributor

Choose a reason for hiding this comment

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

With useCallback and useEffect, you won't really have these problems since they would always use the latest values, and their execution is not dependant on the render function.

I am not sure I follow. If I have a useEffectdeclaring getState as a dependency, I expect useEffect won't be re-run even if the state has changed.

What do you mean by "execution not dependent on the render function" ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Issues can arise in situations like this though

Yes I agree, that was the case I meant in my first comment, and why I am not fond of giving a tool like this helper which can bring bugs but that's just my opinion. What K like about hooks is that if you follow the eslint rules and stick to them you should can this kind of bugs. In the meantime, I hear that performance issues can arise with too many re-renders

This mainly refers to useCallback

Yes again I agree this applies to useCallback but I was more on the fence for this statement about useEffect

Copy link
Contributor

Choose a reason for hiding this comment

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

If this helper brings a lot of value I won't fight over it even if I have concerns

I really like what you suggested in this comment: #1354 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ya. I misspoke putting useEffect in that sentence. The point that I was trying to make with putting in there was that the value in the state doesn't necessarily affect the lifecycle of the effect.

How do you feel about the explicit approach?

const [state, setState] = useState(DEFAULT_STATE);
const stateRef = useRef(state);

const onMouseUp = useCallback(() => {
   const state = stateRef.current;
   ...
}, [])

Copy link
Contributor

Choose a reason for hiding this comment

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

That way it is more self-explanatory and as you said we won't be tempted to use it in places it can cause bugs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. I'll change it to use the explicit version.
I'll also make sure to comment it so that the reader knows why the state is being stored in the ref


export default function ZoomHandler(props) {
const {
events: {mouseMove, mouseDown, mouseUp, mouseLeave},
onZoom,
enableX = true,
enableY = true,
marginLeft = 0,
marginTop = 0,
innerWidth = 0,
innerHeight = 0
} = props;

const [state, setState, getState] = useStateWithGet(DEFAULT_STATE);

const convertArea = useCallback(
area => {
const xScale = getAttributeScale(props, 'x');
const yScale = getAttributeScale(props, 'y');

return {
bottom: yScale.invert(area.bottom),
left: xScale.invert(area.left - marginLeft),
right: xScale.invert(area.right - marginLeft),
top: yScale.invert(area.top)
};
},
[marginLeft, props]
);

const onMouseMove = useCallback(
e => {
const state = getState();
if (!state.brushing) {
return;
}
e.stopPropagation();
e.preventDefault();
const position = getPosition(e);

setState(state => {
const bounds = {
left: enableX
? Math.min(position.x, state.startPosition.x)
: marginLeft,
top: enableY
? Math.min(position.y, state.startPosition.y)
: marginTop,
right: enableX
? Math.max(position.x, state.startPosition.x)
: innerWidth + marginLeft,
bottom: enableY
? Math.max(position.y, state.startPosition.y)
: innerHeight + marginTop
};
return {
...state,
bounds
};
});
},
[
enableX,
enableY,
getState,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The biggest culprit is the fact that onMouseUp needs the bounds which changes every-time the mouse moves which brushing.
Using a separate useState for brushing wouldn't be too bad since that only changes onMouseDown / onMouseUp.

I'm also wary about splitting up the useState calls because then I would need to make 3 calls to state setter functions in both onMouseUp and onMouseDown. I would assume that React would batch these calls and only cause 1 re-render, but I'm not 100% sure. I know that in React Native calling setState with a function actually executes that asynchronously so I lose the ability to reason when the state will be changed.

Copy link
Contributor

@reitlepax reitlepax Jun 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Indeed I did not have in mind the need for onMouse handlers to update the whole state.

That's too bad I thought splitting bounds could solve part of the issue, but if I recall react does not necessarily batch multiple setState calls (I believe they often are batched react-dom, but there is no guarantee it's 100% of the time so we can't rely on it).

Since the goal is to improve perf, splitting the state wouldn't be an improvement so let's drop that suggestion

innerHeight,
innerWidth,
marginLeft,
marginTop,
setState
]
);

const onMouseDown = useCallback(
e => {
e.stopPropagation();
e.preventDefault();
const {x, y} = getPosition(e);

const bounds = {left: x, top: y, right: x, bottom: y};

setState(state => ({
...state,
brushing: true,
bounds,
startPosition: {x, y}
}));
},
[setState]
);

const onMouseUp = useCallback(
e => {
const state = getState();

if (!state.brushing) {
return setState(DEFAULT_STATE);
}

e.stopPropagation();
e.preventDefault();

if (
state.bounds.bottom - state.bounds.top > 5 &&
state.bounds.right - state.bounds.left > 5
) {
onZoom && onZoom(convertArea(state.bounds));
}

setState(DEFAULT_STATE);
},
[convertArea, getState, onZoom, setState]
);

const onMouseLeave = useCallback(() => {
const state = getState();
if (state.brushing) {
setState(DEFAULT_STATE);
}
}, [getState, setState]);

useEffect(() => mouseMove.subscribe(onMouseMove), [mouseMove, onMouseMove]);
useEffect(() => mouseDown.subscribe(onMouseDown), [mouseDown, onMouseDown]);
useEffect(() => mouseUp.subscribe(onMouseUp), [mouseUp, onMouseUp]);
useEffect(() => mouseLeave.subscribe(onMouseLeave), [
mouseLeave,
onMouseLeave
]);

if (!state.brushing) {
return null;
}

const {bounds} = state;
const {opacity, color, className} = props;

return (
<g className={getCombinedClassName(className, 'rv-highlight-container')}>
<rect
className="rv-highlight"
pointerEvents="none"
opacity={opacity}
fill={color}
x={bounds.left}
y={bounds.top}
width={Math.max(0, bounds.right - bounds.left)}
height={Math.max(0, bounds.bottom - bounds.top)}
/>
</g>
);
}
ZoomHandler.requiresSVG = true;

function getPosition(evt) {
const x = evt.nativeEvent.offsetX ?? evt.nativeEvent.pageX;
const y = evt.nativeEvent.offsetY ?? evt.nativeEvent.pageY;
return {x, y};
}
20 changes: 20 additions & 0 deletions packages/react-vis/src/utils/events.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
export class Event {
subscribers = [];

constructor(name) {
this.name = name;
}

fire(...args) {
this.subscribers.forEach(cb => cb(...args));
}

subscribe(callback) {
this.subscribers.push(callback);
return () => this.unsubscribe(callback);
}

unsubscribe(callback) {
this.subscribers = this.subscribers.filter(x => x !== callback);
}
}
26 changes: 23 additions & 3 deletions packages/website/storybook/misc-story.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,19 @@
/* eslint-env node */

import React from 'react';
import React, {useState, useCallback} from 'react';

import {storiesOf} from '@storybook/react';

import {withKnobs, boolean} from '@storybook/addon-knobs/react';
import {withKnobs, boolean, button} from '@storybook/addon-knobs/react';
import {SimpleChartWrapper} from './storybook-utils';
import {generateLinearData} from './storybook-data';

import {LineSeries, ContentClipPath} from 'react-vis';
import {LineSeries, MarkSeries, ContentClipPath, ZoomHandler} from 'react-vis';

const data = generateLinearData({randomFactor: 10});

const highlightData = generateLinearData({});

storiesOf('Misc', module)
.addDecorator(withKnobs)
.addWithJSX('Clip Content', () => {
Expand All @@ -28,4 +30,22 @@ storiesOf('Misc', module)
<LineSeries data={data} style={{clipPath: 'url(#clip)'}} />
</SimpleChartWrapper>
);
})
.addWithJSX('Highlight', () => {
const [zoom, setZoom] = useState();
const onZoom = useCallback(area => {
console.log('zoom', area);
setZoom(area);
}, []);

button('Reset Zoom', () => setZoom(null), 'Zoom');

const xDomain = zoom ? [zoom.left, zoom.right] : undefined;
const yDomain = zoom ? [zoom.bottom, zoom.top] : undefined;
return (
<SimpleChartWrapper xDomain={xDomain} yDomain={yDomain}>
<ZoomHandler opacity={0.2} onZoom={onZoom} />
<MarkSeries data={highlightData} />
</SimpleChartWrapper>
);
});
5 changes: 5 additions & 0 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6603,6 +6603,11 @@ eslint-plugin-prettier@^3.1.3:
dependencies:
prettier-linter-helpers "^1.0.0"

eslint-plugin-react-hooks@^4.0.4:
version "4.0.4"
resolved "https://registry.yarnpkg.com/eslint-plugin-react-hooks/-/eslint-plugin-react-hooks-4.0.4.tgz#aed33b4254a41b045818cacb047b81e6df27fa58"
integrity sha1-rtM7QlSkGwRYGMrLBHuB5t8n+lg=

eslint-plugin-react@^6.7.1:
version "6.10.3"
resolved "https://registry.yarnpkg.com/eslint-plugin-react/-/eslint-plugin-react-6.10.3.tgz#c5435beb06774e12c7db2f6abaddcbf900cd3f78"
Expand Down