Feature/victory brush #190

boygirl · 2017-01-24T01:05:30Z

supporting changes for VictoryBrushContainer and VictoryZoomContainer

This PR:

-Allows VictoryContainer to render either <g> or <svg> depending on the value of the standalone prop

Passes a timer down in context for VictorySharedEvents
Enhances addEvents so that evented components can pick up "parentControllerProps" from parent state (useful for VictoryZoomContainer)
Adds the ability to define callbacks in the events prop that will be called after setState (useful for VictoryZoomContainer resumeAnimation)

DerekMaffett · 2017-01-26T13:07:50Z

src/victory-util/events.js

@@ -148,12 +148,22 @@ export default {
        parseEvent(eventReturn, eventKey);
    };

+    const compileCallbacks = (eventReturn) => {
+      const getCallback = (obj) => isFunction(obj.callback) && obj.callback;


This function doesn't return a constant type. It can return false or a callback function. && technically allows this function but typically I see the final return being used as a boolean, rather than as one of different possible types. I would probably just _.get(obj, 'callback') here instead to get function/undefined instead of function/boolean. Ideally we could enforce that obj.callback has to be a function and then just be able to trust in its type at this point.

DerekMaffett · 2017-01-26T13:11:31Z

src/victory-util/events.js

@@ -148,12 +148,22 @@ export default {
        parseEvent(eventReturn, eventKey);
    };

+    const compileCallbacks = (eventReturn) => {
+      const getCallback = (obj) => isFunction(obj.callback) && obj.callback;
+      const callbacks = Array.isArray(eventReturn) ?


It looks like the uncertainty here is whether eventReturn is a single event object or an array of one or more of them. Why don't we just coerce this into an array initially and then we can map this without question?

DerekMaffett · 2017-01-26T13:15:47Z

src/victory-util/events.js

+      const getCallback = (obj) => isFunction(obj.callback) && obj.callback;
+      const callbacks = Array.isArray(eventReturn) ?
+        eventReturn.map((evtObj) => getCallback(evtObj)) : [getCallback(eventReturn)];
+      const callbackArray = callbacks.filter((callback) => callback !== false);


Even if we change it to undefined callbacks instead of false, this is just callbacks.filter(_.identity). Which is the same as _.compact(callbacks)

DerekMaffett · 2017-01-26T13:26:29Z

src/victory-util/events.js

+      const callbacks = Array.isArray(eventReturn) ?
+        eventReturn.map((evtObj) => getCallback(evtObj)) : [getCallback(eventReturn)];
+      const callbackArray = callbacks.filter((callback) => callback !== false);
+      return callbackArray.length ?


Why not return a consistent type here? Triggering an empty list of callbacks will just result in nothing happening, which seems like a generally appropriate behavior. Is there some kind of special behavior for the 0 callbacks case that needs to be considered post-compile?

DerekMaffett · 2017-01-26T13:36:44Z

src/victory-util/add-events.js

      this.getSharedEventState = sharedEvents && isFunction(sharedEvents.getEventState) ?
        sharedEvents.getEventState : () => undefined;
+      this.baseProps = this.getBaseProps(props);
+      this.dataKeys = Object.keys(this.baseProps).filter((key) => key !== "parent");


I'd probably use reject here ("Reject parent keys" rather than "use keys that aren't parent"). It reads easier to me. Not sure if you're into using both reject and filter though, or if you're trying to minimize dependency on lodash.

DerekMaffett · 2017-01-26T13:37:03Z

src/victory-container/victory-container.js

+    const { title, desc, portalComponent, className, standalone } = props;
+    return standalone ?
+      (
+        <svg {...svgProps} style={style} className={className}>


https://facebook.github.io/react/docs/react-api.html#createelement

Since only the type is different here, this could be refactored by using React.createElement directly instead of JSX and then just changing the type argument based on the value of standalone.

DerekMaffett · 2017-01-26T13:46:45Z

src/victory-util/add-events.js

+      const baseParentProps = defaults({}, parentState, sharedParentState);
+      const parentPropsList = baseParentProps.parentControlledProps;
+      const parentProps = parentPropsList ? pick(baseParentProps, parentPropsList) : {};
+      const modifiedProps = defaults({}, parentProps, props);


General question - it looks like defaults acts basically as a reverse-priority merge, but I also see merge used in this repo. Is there a standard you prefer?

DerekMaffett · 2017-01-26T13:51:56Z

src/victory-util/selection.js

@@ -12,13 +12,30 @@ export default {
    }
  },

+  getTransformationMatrix(svg) {


I'll have to learn more about svg before I can give a good review of this section.

boygirl added 7 commits January 18, 2017 16:19

safety commit

b75b01d

shared events should pass a timer down in context

607c29f

pass context as an argument to event handlers

3429ca7

add ability to define callbacks for setState from event returns

02f2b92

use domain when provided

3e5aacd

pick up parentControlledProps from state

d950b34

container renders g or svg

fa3255b

boygirl merged commit 5ef16eb into master Jan 30, 2017

boygirl deleted the feature/victory-brush branch January 30, 2017 17:21

DerekMaffett reviewed Jan 31, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/victory brush #190

Feature/victory brush #190

boygirl commented Jan 24, 2017 •

edited

Loading

DerekMaffett Jan 26, 2017

DerekMaffett Jan 26, 2017

DerekMaffett Jan 26, 2017

DerekMaffett Jan 26, 2017

DerekMaffett Jan 26, 2017

DerekMaffett Jan 26, 2017

DerekMaffett Jan 26, 2017

DerekMaffett Jan 26, 2017

Feature/victory brush #190

Feature/victory brush #190

Conversation

boygirl commented Jan 24, 2017 • edited Loading

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

boygirl commented Jan 24, 2017 •

edited

Loading