Created store and reducers #1108

vera-liu · 2016-09-14T19:17:35Z

Four objects in store: TimeFilter, GroupBy, Sql and Filter
For the four object-stores above, action can be fired once the query button is pressed
For the stores that are not contained in objects (datasourceId, vizType, columns, ordering, etc.) the actions will be fired once the field is changed, for instance, the user could see updated visualization once he selects a different viz_type without clicking the query button.

ascott · 2016-09-14T20:56:51Z

caravel/assets/javascripts/explorev2/reducers.js

+  col: null,
+};
+
+export const initialState = {


i think initialState should go in the store

You mean load it in store?
https://github.com/vera-liu/caravel/blob/a9f6cd153cfd0657d24841936e82b732a45eaace/caravel/assets/javascripts/explorev2/index.jsx#L21

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.

ascott · 2016-09-14T21:11:47Z

this is looking really good!

i think now would be a good time to add tests for the reducers and actions.
there are some examples here: http://redux.js.org/docs/recipes/WritingTests.html#action-creators

this is a good example of logging your state to test in browser, to make sure your actions are returning the new state correctly: http://redux.js.org/docs/basics/Store.html#dispatching-actions

mistercrunch · 2016-09-15T17:14:01Z

caravel/assets/javascripts/explorev2/reducers.js

+  filters: [defaultFilter],
+};
+
+function addToArr(state, arrKey, obj) {


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.

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

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

mistercrunch · 2016-09-15T17:19:08Z

caravel/assets/javascripts/explorev2/actions.js

+  return { type: SET_SQL, sql };
+}
+
+export function addFilter(filter) {


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?

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

ascott · 2016-09-20T01:28:05Z

caravel/assets/package.json

@@ -112,6 +112,7 @@
    "react-addons-test-utils": "^0.14.8",
    "react-dom": "^0.14.8",
    "style-loader": "^0.13.0",
+    "tape": "^4.6.0",


we should remove this dependency since we will stick with mocha for testing.

ascott · 2016-09-20T01:28:43Z

caravel/assets/spec/javascripts/explore/components/actions.spec.js

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


this file should be located in caravel/assets/spec/javascripts/explore/actions/ and be renamed to actions_spec.js

ascott · 2016-09-20T01:29:40Z

caravel/assets/javascripts/explorev2/actions.js

@@ -0,0 +1,71 @@
+export const SET_DATASOURCE = 'SET_DATASOURCE';


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

ascott · 2016-09-20T01:29:59Z

caravel/assets/javascripts/explorev2/reducers.js

@@ -0,0 +1,89 @@
+import shortid from 'shortid';


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

ascott · 2016-09-20T01:30:10Z

caravel/assets/javascripts/explorev2/store.js

@@ -0,0 +1,39 @@
+import shortid from 'shortid';


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

ascott · 2016-09-20T03:35:09Z

caravel/assets/spec/javascripts/explore/components/actions.spec.js

+import shortid from 'shortid';
+import * as actions from '../../../../javascripts/explorev2/actions';
+import { initialState } from '../../../../javascripts/explorev2/store';
+import { exploreReducer } from '../../../../javascripts/explorev2/reducers';


the import name and file name should be the same according to our style guide: https://github.com/airbnb/javascript#naming--filename-matches-export

ascott · 2016-09-20T23:07:51Z

caravel/assets/javascripts/SqlLab/reducers.js

@@ -1,6 +1,7 @@
 import shortid from 'shortid';
 import * as actions from './actions';
 import { now } from '../modules/dates';
+import * as utils from '../../utils/reducerUtils';


would be nice to use destructuring here: so you don't have to use utils.addToArr elsewhere.
const { addToArr, removeFromArr, alterInArr, alterInObject } from '../../utils/reducerUtils';

ascott · 2016-09-20T23:42:40Z

caravel/assets/javascripts/explorev2/index.jsx

+import { exploreReducer } from './reducers/exploreReducer';
+import persistState from 'redux-localstorage';
+
+let enhancer = compose(persistState());


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

ascott · 2016-09-20T23:45:15Z

caravel/assets/javascripts/explorev2/index.jsx

-  />,
+  <Provider store={store}>
+    <ExploreViewContainer
+      data={bootstrapData}


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

so above we probably want to do

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

vera-liu · 2016-09-21T18:20:53Z

Made modifications based on all of the comments, the python tests are failing, is it expected and safe to merge? @ascott

ascott · 2016-09-21T18:35:59Z

yeah, this is safe to merge since we are just merging to airbnb:alanna-explore-v2-main and not to master. for some reason it seems tests fail on PR's that are not to master. @mistercrunch @bkyryliuk do u have any ideas on this?

in the mean time @vera-liu let's merge!

ascott · 2016-09-21T18:47:04Z

caravel/assets/javascripts/SqlLab/index.jsx

 import { connect, Provider } from 'react-redux';

 import { initialState, sqlLabReducer } from './reducers';
-import persistState from 'redux-localstorage';
+import { enhancer } from '../../utils/common';


i wonder if this should go in a reduxUtils.js file rather than just common utils...

sorry merged in too early ;( I'll fix this in my next refactor_explore PR

np, just a thought for another PR 👌

* Created store and reducers * Added spec * Modifications based on comments

…1205) * create structure for new forked explore view (#1099) * create structure for new forked explore view * update component name * add bootstrap data pattern * remove console.log * Associate version to entry files (#1060) * Associate version to entry files * Modified path joins for configs * Made changes based on comments * Created store and reducers (#1108) * Created store and reducers * Added spec * Modifications based on comments * Explore control panel components: Chart control, Time filter, SQL, GroupBy and Filters * Modifications based on comments

* Explore control panel - Chart control, TimeFilter, GroupBy, Filters (#1205) * create structure for new forked explore view (#1099) * create structure for new forked explore view * update component name * add bootstrap data pattern * remove console.log * Associate version to entry files (#1060) * Associate version to entry files * Modified path joins for configs * Made changes based on comments * Created store and reducers (#1108) * Created store and reducers * Added spec * Modifications based on comments * Explore control panel components: Chart control, Time filter, SQL, GroupBy and Filters * Modifications based on comments * accommodate old and new explore urls * move bootstrap data up in scope * fix code climate issues * fix long lines * fix syntax error

…pache#1205) * create structure for new forked explore view (apache#1099) * create structure for new forked explore view * update component name * add bootstrap data pattern * remove console.log * Associate version to entry files (apache#1060) * Associate version to entry files * Modified path joins for configs * Made changes based on comments * Created store and reducers (apache#1108) * Created store and reducers * Added spec * Modifications based on comments * Explore control panel components: Chart control, Time filter, SQL, GroupBy and Filters * Modifications based on comments

* Explore control panel - Chart control, TimeFilter, GroupBy, Filters (#1205) * create structure for new forked explore view (#1099) * create structure for new forked explore view * update component name * add bootstrap data pattern * remove console.log * Associate version to entry files (#1060) * Associate version to entry files * Modified path joins for configs * Made changes based on comments * Created store and reducers (#1108) * Created store and reducers * Added spec * Modifications based on comments * Explore control panel components: Chart control, Time filter, SQL, GroupBy and Filters * Modifications based on comments * Added access check + Druid in endpoint * pull grains to constants * Switch explore.html to old version

…pache#1205) * create structure for new forked explore view (apache#1099) * create structure for new forked explore view * update component name * add bootstrap data pattern * remove console.log * Associate version to entry files (apache#1060) * Associate version to entry files * Modified path joins for configs * Made changes based on comments * Created store and reducers (apache#1108) * Created store and reducers * Added spec * Modifications based on comments * Explore control panel components: Chart control, Time filter, SQL, GroupBy and Filters * Modifications based on comments

* create structure for new forked explore view (#1099) * create structure for new forked explore view * update component name * add bootstrap data pattern * remove console.log * Created store and reducers (#1108) * Created store and reducers * Added spec * Modifications based on comments * do use bootstrap data for now * don't deal with bootstrap data for now * use victory as a base * import fake line data, add fake panels, make chart fixed * add fetch support * get slice data from json endpoint * render chart with slicejson * update chart and label demo * remove fetch config * remove dummy control panels * should be a func * make TimeSeriesLineChart * add a comment * inner height for height * don't need fetch yet * trailing comma breaks in package json * pass in viz data from props * add style sheet * set height on explore container * add legend * make chart responsive to window resize * can't use head_css in template bc overrides head_css in basic * fix linting * break labelItem into own SFC, make legend SFC * add propTypes and fix linter

Created store and reducers

a9f6cd1

ascott reviewed Sep 14, 2016

View reviewed changes

vera-liu force-pushed the vliu_refactor_explore branch from 808bcbf to a45f617 Compare September 20, 2016 00:57

mistercrunch requested changes Sep 20, 2016

View reviewed changes

ascott reviewed Sep 20, 2016

View reviewed changes

vera-liu force-pushed the vliu_refactor_explore branch 2 times, most recently from 67a69e4 to b9648b9 Compare September 20, 2016 21:04

Added spec

ad318ee

vera-liu force-pushed the vliu_refactor_explore branch from b9648b9 to ad318ee Compare September 20, 2016 21:06

ascott reviewed Sep 20, 2016

View reviewed changes

vera-liu mentioned this pull request Sep 20, 2016

[explore] stores and actions for refactoring explore view #1155

Closed

ascott reviewed Sep 20, 2016

View reviewed changes

vera-liu force-pushed the vliu_refactor_explore branch from e3c339e to 1fe90e8 Compare September 21, 2016 17:26

Modifications based on comments

352ae51

vera-liu force-pushed the vliu_refactor_explore branch from 1fe90e8 to 352ae51 Compare September 21, 2016 17:41

vera-liu merged commit dcfb604 into apache:alanna-explore-v2-main Sep 21, 2016

ascott reviewed Sep 21, 2016

View reviewed changes

vera-liu added a commit to vera-liu/caravel that referenced this pull request Sep 27, 2016

Created store and reducers (apache#1108)

2353549

* Created store and reducers * Added spec * Modifications based on comments

ym97 mentioned this pull request Oct 27, 2016

Explore control panel - Chart control, TimeFilter, GroupBy, Filters (… #1447

Closed

pinkythalli97 mentioned this pull request Oct 29, 2016

Explore control panel - Chart control, TimeFilter, GroupBy, Filters (… #1476

Closed

zhaoyongjie pushed a commit to zhaoyongjie/incubator-superset that referenced this pull request Nov 17, 2021

chore: removes react-bootstrap (apache#1108)

ad7e829

zhaoyongjie pushed a commit to zhaoyongjie/incubator-superset that referenced this pull request Nov 24, 2021

chore: removes react-bootstrap (apache#1108)

7aaf017

zhaoyongjie pushed a commit to zhaoyongjie/incubator-superset that referenced this pull request Nov 25, 2021

chore: removes react-bootstrap (apache#1108)

0edf1ee

zhaoyongjie pushed a commit to zhaoyongjie/incubator-superset that referenced this pull request Nov 26, 2021

chore: removes react-bootstrap (apache#1108)

0eaa79d

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.5.0 labels Feb 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Created store and reducers #1108

Created store and reducers #1108

vera-liu commented Sep 14, 2016 •

edited

Loading

ascott Sep 14, 2016

vera-liu Sep 14, 2016 •

edited

Loading

ascott Sep 14, 2016

vera-liu Sep 14, 2016

ascott commented Sep 14, 2016 •

edited

Loading

mistercrunch Sep 15, 2016 •

edited

Loading

ascott Sep 20, 2016 •

edited

Loading

vera-liu Sep 20, 2016

ascott Sep 20, 2016

mistercrunch Sep 15, 2016

ascott Sep 20, 2016

ascott Sep 20, 2016

vera-liu Sep 20, 2016

ascott Sep 20, 2016

vera-liu Sep 20, 2016

ascott Sep 20, 2016

vera-liu Sep 20, 2016

ascott Sep 20, 2016

vera-liu Sep 20, 2016

ascott Sep 20, 2016

vera-liu Sep 20, 2016

ascott Sep 20, 2016

vera-liu Sep 20, 2016

ascott Sep 20, 2016 •

edited

Loading

ascott Sep 20, 2016

ascott Sep 20, 2016

ascott Sep 20, 2016

vera-liu commented Sep 21, 2016

ascott commented Sep 21, 2016

ascott Sep 21, 2016

vera-liu Sep 21, 2016

ascott Sep 21, 2016

		@@ -0,0 +1,71 @@
		export const SET_DATASOURCE = 'SET_DATASOURCE';

Created store and reducers #1108

Created store and reducers #1108

Conversation

vera-liu commented Sep 14, 2016 • edited Loading

Choose a reason for hiding this comment

vera-liu Sep 14, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ascott commented Sep 14, 2016 • edited Loading

mistercrunch Sep 15, 2016 • edited Loading

Choose a reason for hiding this comment

ascott Sep 20, 2016 • 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

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

ascott Sep 20, 2016 • 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

vera-liu commented Sep 21, 2016

ascott commented Sep 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vera-liu commented Sep 14, 2016 •

edited

Loading

vera-liu Sep 14, 2016 •

edited

Loading

ascott commented Sep 14, 2016 •

edited

Loading

mistercrunch Sep 15, 2016 •

edited

Loading

ascott Sep 20, 2016 •

edited

Loading

ascott Sep 20, 2016 •

edited

Loading