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

Filter and Formula Data Transforms on DataTable #397

Merged
merged 47 commits into from Sep 12, 2016

Conversation

Projects
None yet
2 participants
@mattwchun
Copy link
Contributor

mattwchun commented Aug 19, 2016

Description of the problem this pull request fixes
This will add the feature to do Filter and Formula data transformations on the data in the DataTable component.

Changes proposed in this pull request:

  • filter data transforms in the DataTable
  • formula data transforms in the DataTable

** If submitting code for review: **

This PR includes:

  • Tests
  • functional comments
  • high level description of any new components

Steps to functionally test this:

…data transformations to the DataTable

mattwchun added some commits Aug 19, 2016

Added filter svg and made corresponding changes to assets.js. In the …
…FilterField component I started writing out what will be necessary to include
Added FilterField component to the HoverField component after the Sor…
…tField. Also know the onclick function is correct and will just need to dispatch a action for filter like sort
Added filter dataset reducer, exporter filter function which appends …
…the filter vega code to the spec, and added FILTER_DATASET to the list of the actions that invalidate the vega spec. Also added to the exporter dataset function to append the filter vega code if _filter property is set in the store.
Wrote up the reducer and action creator to display the textbox in the…
… PipelineInspector. This isn't working for some reason. When click filter icon, a new pipeline seems to be created
Added the property autocomplete textbox when filter icon clicked. In …
…pipeline modal, clicking on the filter icon will throw an error
Moved the expression textbox into its own component and got the onCha…
…nge action event working in the expression textbox component. DataTable component uses the expression textbox
Dispatching action correctly with the text in the expression textbox …
…to be appended to the vega spec. Now just have to figure out when the expression is valid and okay to append to the vega spec
Created new formula field component for the formula vega data transfo…
…rm. Added formula icon component to the HoverField. Shows the expression textbox but will need to show 2 expression textboxes for the field and expression in the formula data transform

@mattwchun mattwchun changed the title Started creating new component for FilterField which will add filter … Filter and Formula Data Transforms on DataTable Sep 1, 2016

module.exports = {
connected: connect(mapStateToProps, mapDispatchToProps)(ExpressionTextbox),
disconnected: ExpressionTextbox
}

This comment has been minimized.

@arvind

arvind Sep 1, 2016

Member

As we add more transforms, it's likely that each of them will have their own special inspector interface. So let's put these files under src/js/components/pipelines/transforms (and rename this particular file to Filter.jsx as being specific to the Filter UI?).

},

render: function() {
return (<Icon onClick={this.showTextbox} glyph={assets.filter} width="10" height="10" />);

This comment has been minimized.

@arvind

arvind Sep 1, 2016

Member

If you rebase to the latest lyra2, you'll see how I've introduced tooltips to these icons (e.g., with FieldType). It would be good to add a tooltip to each of the new buttons you introduce as well.

}

if (action.type === ACTIONS.FILTER_DATASET) {
state = setIn(state, action.id + '._filter',

This comment has been minimized.

@arvind

arvind Sep 1, 2016

Member

If we store the filter expression under ._filter (and formula under ._formula), the user can only specify a single filter and formula expression per dataset. Instead, perhaps we want to have more general addTransform and updateTransform actions?

Both of these would take a dsId and a def, which is the Vega definition for a particular transform (e.g., {"type": "filter", "test": ""}). The reducer would then append the def to a dataset's transform array. updateTransform would also have to take an additional idx to identify which element in the transform array is being updated.

This structure would also simplify the exporter logic as we don't need to do anything special to export the transforms. They'd be exported directly as part of a dataset.

Immutable.fromJS({
show: action.show,
time: action.time
}));

This comment has been minimized.

@arvind

arvind Sep 1, 2016

Member

I'm not sure this logic should be part of the store. Instead, because it's so specific to a particular React component, this may be one of the rare cases that we want to use the internal state of the component.

@arvind

This comment has been minimized.

Copy link
Member

arvind commented Sep 1, 2016

Based on my last comment of moving the "show textbox" logic out of the store, here's how I imagine the flow operating. We'll use a running example of filtering by Horsepower from the cars dataset.

When a user clicks the filter icon, an addTransform(dsId, {"type": "filter", "test": "datum.Horsepower "}) action is fired. Note, we pre-populate the expression with the field that was clicked on, so it should appear in the AutoComplete textbox. The reducer adds this transform to the corresponding dataset, and a FilterTransform component now appears above the DataTable.

The FilterTransform component maintains an internal state that determines whether it's expanded (i.e., showing the autocomplete textbox) or contracted (showing just a button). An internal timer (window.setTimeout) keeps track of when to flip from expanded to contracted. When the user types in the textbox, the timer is cleared and restarts. This logic will likely be shared across all transform components, so we may want to create a parent Transform component within which we instantiate the particular "expanded" state. Here's a sketch:

Transform.jsx

getInitialState: function() {
  return {expanded: true};
},

timer: null,

resetTimer: function() {
  var that = this;
  window.clearTimeout(this.timer);
  this.timer = window.setTimeout(function() { 
    that.setState({expanded: false});
  }, 10000);
},

render: function() {
  return this.state.expanded ? this.props.children : (<div>...</div>);
}

FilterTransform.jsx

<Transform>
    <Property type="autocomplete" ... />
</Transform>

mattwchun added some commits Sep 2, 2016

Changed directory structure a little in pipeline components. Moved al…
…l the transform components into the new transform folder
Renamed icon components in pipeline/transform. Merged in the updates …
…and fixed the corresponding merge conflicts
Got the addTransform action and corresponding reducer working. Still …
…need to remove old expression Textbox reducer and action code.
Restructured folder structure by creating icon and inspector folders …
…in the transform folder for better future data transformation organization
Started the transform inspector which in the wrapper component that d…
…etermines which inspectors or expression textboxes to show up. Also moved expressionTextbox component to the transform folder and editted paths accordingly
Finished implementing the 10 second autoclose expression textbox but …
…still need to test it. This expression textbox component will go within each of data transform inspectors
time: time
};
}

This comment has been minimized.

@arvind

arvind Sep 7, 2016

Member

I don't think these two action creators are used anymore? Be sure to clean them up (along with associated variables and exports).

mattwchun added some commits Sep 7, 2016

Added a specId property that represents the index of the transform in…
… the store array of transforms to the sub-inspectors. Only thing that is not working is the render is not called after mapstatetoprops is called even though the array is changed in the Inspector component
Added the functionality so that the expression textbox hides after 10…
… seconds. However now when the user reclicks since the Inspector component is not re rendering after the change in the transform array, the expression box does not show up again
Added shouldComponentUpdate to check deep equality of transform array…
… from the store but does not work because it is called only when the render is called. I need do this check in mapStatetoProps and tell the component that the prop changed even though the array reference address is the same
Fixed most of the lint style errors in expression textbox. Only error…
… remaining is empty components are self-closing
spec.transform.push(element);
});
}

This comment has been minimized.

@arvind

arvind Sep 8, 2016

Member

If you store the transforms under dataset.transforms (rather than ._transforms) you won't need this code anymore -- the transforms will get automatically exported when we call .toJS() earlier in this method.

var Inspector = React.createClass({
propTypes: {
dsId: React.PropTypes.number,
transforms: React.PropTypes.array

This comment has been minimized.

@arvind

arvind Sep 8, 2016

Member

transforms should be an ImmutableList in the store (and marked here as a React.PropTypes.instanceof(Immutable.List)).

transforms: React.PropTypes.array
},

shouldComponentUpdate: function(nextProps, nextState) {

This comment has been minimized.

@arvind

arvind Sep 8, 2016

Member

Because our store uses ImmutableJS, you don't need this method to test for deep equality in the passed in properties. Instead, ImmutableJS will produce a new ImmutableList for the transforms property if it is ever updated, which will have a new reference and thus trigger a re-render.

transforms.push(action.transformSpec);
}

state = setIn(state, action.id + '._transforms', transforms);

This comment has been minimized.

@arvind

arvind Sep 8, 2016

Member

Aha, part of the issues you're seeing is because transforms is a plain JavaScript array and then yes you're right, the reference to the array stays the same and so React never re-renders the component. Instead, as we do elsewhere in this file, you'll want to call Immutable.fromJS to convert a JavaScript array back to its equivalent ImmutableJS primitive before putting it back in the store.

@@ -38,6 +39,41 @@ function datasetsReducer(state, action) {
}));
}

if (action.type === ACTIONS.FILTER_DATASET) {

This comment has been minimized.

@arvind

arvind Sep 8, 2016

Member

This code path isn't necessary anymore.

// if transform exist already
transforms[index] = action.newSpec;
}
state = setIn(state, action.id + '._transforms', transforms);

This comment has been minimized.

@arvind

arvind Sep 8, 2016

Member

When editing, you can probably call setIn using the passed in index. For example, return setIn(state, action.id + '.transforms.' + index, Immutable.fromJS(action.newSpec)).

@arvind arvind merged commit ca8f194 into lyra2 Sep 12, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@arvind arvind deleted the FilterTransform branch Sep 12, 2016

@arvind arvind removed the in progress label Sep 12, 2016

@arvind

This comment has been minimized.

Copy link
Member

arvind commented Sep 12, 2016

Great work @mattwchun! You may want to take a look at the changes I made to this as some of them will be applicable to your next task. The commit diffs make it look like a more major change than it actually is. Besides renaming some of the actions and components to fit with our scheme more, and removing one level of nesting from the transforms component directory, the change flips how the transform components instantiate each other.

Whereas previously you had an ExpressionTextbox component (now renamed to TransformInspector) that each transform's inspector called, I inverted this logic (i.e., TransformInspector creates a Filter or Formula inspector as needed). This allows us to share more common code in the TransformInspector (both the timer and connected action code) and pass that down to each individual transform via props. Keeps things simpler, with fewer connected components.

Finally, the formula transform should also update the dataset's schema as necessary. In 9279730, prior to merging this PR, I moved dataset schema definitions into the store. So updating the schema should be much easier now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.