Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
71 changes: 71 additions & 0 deletions caravel/assets/javascripts/explorev2/actions.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
export const SET_DATASOURCE = 'SET_DATASOURCE';

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

let's move this to caravel/assets/javascripts/explorev2/actions

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done:)

export const SET_VIZTYPE = 'SET_VIZTYPE';
export const SET_TIME_FILTER = 'SET_TIME_FILTER';
export const SET_GROUPBY = 'SET_GROUPBY';
export const ADD_COLUMN = 'ADD_COLUMN';
export const REMOVE_COLUMN = 'REMOVE_COLUMN';
export const ADD_ORDERING = 'ADD_ORDERING';
export const REMOVE_ORDERING = 'REMOVE_ORDERING';
export const SET_TIME_STAMP = 'SET_TIME_STAMP';
export const SET_ROW_LIMIT = 'SET_ROW_LIMIT';
export const TOGGLE_SEARCHBOX = 'TOGGLE_SEARCHBOX';
export const SET_SQL = 'SET_SQL';
export const ADD_FILTER = 'ADD_FILTER';
export const SET_FILTER = 'SET_FILTER';
export const REMOVE_FILTER = 'REMOVE_FILTER';

export function setDatasource(datasourceId) {
return { type: SET_DATASOURCE, datasourceId };
}

export function setVizType(vizType) {
return { type: SET_VIZTYPE, vizType };
}

export function setTimeFilter(timeFilter) {
return { type: SET_TIME_FILTER, timeFilter };
}

export function setGroupBy(groupBy) {
return { type: SET_GROUPBY, groupBy };
}

export function addColumn(column) {
return { type: ADD_COLUMN, column };
}

export function removeColumn(column) {
return { type: REMOVE_COLUMN, column };
}

export function addOrdering(ordering) {
return { type: ADD_ORDERING, ordering };
}

export function removeOrdering(ordering) {
return { type: REMOVE_ORDERING, ordering };
}

export function setTimeStamp(timeStampFormat) {
return { type: SET_TIME_STAMP, timeStampFormat };
}

export function setRowLimit(rowLimit) {
return { type: SET_ROW_LIMIT, rowLimit };
}

export function toggleSearchBox(searchBox) {
return { type: TOGGLE_SEARCHBOX, searchBox };
}

export function setSQL(sql) {
return { type: SET_SQL, sql };
}

export function addFilter(filter) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

After a bit of experience on Redux, I think passing objects to action creators isn't a great pattern because there's no place where the object structure is defined except for where it's being created. If it's being created in only one place in the codebase it isn't too bad, but if many places in the code need to create the object there's no central place for constructor-type logic.

After a bit of thinking I now think that action creator is the right place to act as a constructor. Meaning the action creator would detail which param the object has, and implement constructor logic if any.

@ascott , thoughts?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

i'm not sure i get what you are suggesting here... could you clarify?

return { type: ADD_FILTER, filter };
}

export function removeFilter(filter) {
return { type: REMOVE_FILTER, filter };
}
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ export default class ExploreViewContainer extends React.Component {
<div className="col-sm-3">
<QueryAndSaveButtons
canAdd="True"
onQuery={() => { console.log('clicked query') }}
onQuery={() => { console.log('clicked query'); }}
/>
<br /><br />
<ControlPanelsContainer />
Expand Down
23 changes: 20 additions & 3 deletions caravel/assets/javascripts/explorev2/index.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,29 @@ import React from 'react';
import ReactDOM from 'react-dom';
import ExploreViewContainer from './components/ExploreViewContainer';

import { compose, createStore } from 'redux';
import { Provider } from 'react-redux';

const exploreViewContainer = document.getElementById('js-explore-view-container');
const bootstrapData = exploreViewContainer.getAttribute('data-bootstrap');


import { initialState, exploreReducer } from './reducers';
import persistState from 'redux-localstorage';

let enhancer = compose(persistState());

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

i think i've seen this pattern in sql-lab too, would be great to add to redux utils.

if (process.env.NODE_ENV === 'dev') {
enhancer = compose(
persistState(), window.devToolsExtension && window.devToolsExtension()
);
}
let store = createStore(exploreReducer, initialState, enhancer);

ReactDOM.render(
<ExploreViewContainer
data={bootstrapData}
/>,
<Provider store={store}>
<ExploreViewContainer
data={bootstrapData}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

is there any data from the server in bootstrapData that can be used to bootstrap the initial state? i think we get vizType and datasourceID

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

so above we probably want to do

const bootstrappedState = Object.assign(initialState, {
  datasourceId: bootstrapData.datasourceId,
  vizType: bootstrapData.vizType
});

const store = createStore(exploreReducer, bootstrappedState, enhancer);

/>
</Provider>,
exploreViewContainer
);
127 changes: 127 additions & 0 deletions caravel/assets/javascripts/explorev2/reducers.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,127 @@
import shortid from 'shortid';

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

let's move this to caravel/assets/javascripts/explorev2/reducers

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done:)

import * as actions from './actions';

const defaultTimeFilter = {
timeColumn: null,
timeGrain: null,
since: null,
until: null,
};

const defaultGroupBy = {
groupByColumn: [],
metrics: [],
};

const defaultSql = {
where: '',
having: '',
};

const defaultFilter = {
id: shortid.generate(),
eq: null,
op: 'in',
col: null,
};

export const initialState = {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

i think initialState should go in the store

@vera-liu vera-liu Sep 14, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

i mean put initial state in a file called store.js

then rather than importing initial state from ./reducers.js, import from store.js

the way you are creating the store looks good.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Got it!

datasourceId: null,
vizType: null,
timeFilter: defaultTimeFilter,
groupBy: defaultGroupBy,
columns: [],
orderings: [],
timeStampFormat: null,
rowLimit: null,
searchBox: false,
SQL: defaultSql,
filters: [defaultFilter],
};

function addToArr(state, arrKey, obj) {

@mistercrunch mistercrunch Sep 15, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

All of these helper functions are copy-pasted from SqlLab/reducer.jsx, let's refactor them in some reducerUtils.jsx module. Copy-pasting code is bad because it then needs to be maintained in multiple places.

@ascott ascott Sep 20, 2016

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

yes, definitely great idea to share these utils. best place for them would be caravel/assets/javascripts/utils/, then all reducers can import from there.

reducerUtils.js

export function addToArr(state, arrKey, obj) {
  ...
}

export function someOtherUtil(state, arrKey, obj) {
  ...
}

import { addToArr, someOtherUtil } from '../utils/reducerUtils.js

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done:)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

great! looks like you just need to push your new changes...

const newState = {};
newState[arrKey] = [...state[arrKey], obj];
return Object.assign({}, state, newState);
}

function removeFromArr(state, arrKey, obj) {
const newArr = [];
state[arrKey].forEach((arrItem) => {
if (!(obj === arrItem)) {
newArr.push(arrItem);
}
});
return Object.assign({}, state, { [arrKey]: newArr });
}

function addToObjArr(state, arrKey, obj) {
const newObj = Object.assign({}, obj);
if (!newObj.id) {
newObj.id = shortid.generate();
}
const newState = {};
newState[arrKey] = [...state[arrKey], newObj];
return Object.assign({}, state, newState);
}

function removeFromObjArr(state, arrKey, obj, idKey = 'id') {
const newArr = [];
state[arrKey].forEach((arrItem) => {
if (!(obj[idKey] === arrItem[idKey])) {
newArr.push(arrItem);
}
});
return Object.assign({}, state, { [arrKey]: newArr });
}

export const exploreReducer = function (state, action) {
const actionHandlers = {
[actions.SET_DATASOURCE]() {
return Object.assign({}, state, { datasourceId: action.datasourceId });
},
[actions.SET_VIZTYPE]() {
return Object.assign({}, state, { vizType: action.vizType });
},
[actions.SET_TIMEFILTER]() {
return Object.assign({}, state, { timeFilter: action.timeFilter });
},
[actions.SET_GROUPBY]() {
return Object.assign({}, state, { groupBy: action.groupBy });
},
[actions.ADD_COLUMN]() {
return addToArr(state, 'columns', action.column);
},
[actions.REMOVE_COLUMN]() {
return removeFromArr(state, 'columns', action.column);
},
[actions.ADD_ORDERING]() {
return addToArr(state, 'orderings', action.ordering);
},
[actions.REMOVE_ORDERING]() {
return removeFromArr(state, 'orderings', action.ordering);
},
[actions.SET_TIME_STAMP]() {
return Object.assign({}, state, { timeStampFormat: action.timeStampFormat });
},
[actions.SET_ROW_LIMIT]() {
return Object.assign({}, state, { rowLimit: action.rowLimit });
},
[actions.TOGGLE_SEARCHBOX]() {
return Object.assign({}, state, { searchBox: action.searchBox });
},
[actions.SET_SQL]() {
return Object.assign({}, state, { SQL: action.sql });
},
[actions.ADD_FILTER]() {
return addToObjArr(state, 'filters', action.filter);
},
[actions.REMOVE_FILTER]() {
return removeFromObjArr(state, 'filters', action.filter);
},
};
if (action.type in actionHandlers) {
return actionHandlers[action.type]();
}
return state;
};